Fix assistant text cut mid-word and repeated in mobile chat#905
Conversation
`chat.getTranscript` rebuilds an assistant message from its streamed `text` fragments. It decided two fragments were contiguous only when the immediately preceding envelope was the same message, and joined them with "\n\n" otherwise. Ephemeral chrome the renderers never draw — activity hints, subagent progress, token usage — sat between deltas constantly, so the separator landed inside words: "no new mod\n\nifier chain needed:". Desktop never showed it because it renders the envelope stream directly and concatenates adjacent text rows. iOS holds both the live fragments and this canonical text and reconciles them, so the mismatch defeated its overlap heuristics and it fell back to concatenation — rendering the message twice. Across the last 60 production transcripts this produced 1101 mid-word splices, and a second bug dropped 4181 whitespace-only fragments (word gaps and markdown hard breaks), gluing words together. The rebuild now guarantees one invariant: canonical text is the verbatim concatenation of a message's fragments. - Fragments sharing a provider messageId always concatenate. The event carries no block-level identity, so ADE cannot tell "new paragraph" from "next delta" and must not guess. - Only rendered content ends an assistant run. `isTranscriptContentEvent` is an allowlist of content types, so a future event type defaults to not breaking a run — losing a paragraph break rather than corrupting a word. - A whitespace-only fragment is a real delta and is dropped only when there is no run for it to continue. Extracted to chatTranscriptEntries.ts with unit tests, including a property test that splits a message at random points and asserts byte-exact rebuild. Verified against 4668 message-keyed entries in 60 production transcripts: zero mismatches. On iOS, `mergeWorkAssistantText` adds the matching guard — an envelope with no sequence came from chat.getTranscript and is a complete message, so it may replace the accumulated text but is never concatenated onto it. Co-Authored-By: Claude <noreply@anthropic.com>
The first pass keyed assistant streams on messageId alone and treated every message-keyed fragment as verbatim. Both were wrong: - Codex inverts the id fields — messageId is a per-turn UUID and itemId is the provider message id — so 17 messageIds in the last 60 transcripts spanned two itemIds and would have concatenated two distinct messages. Identity is now the messageId/itemId pair, joined with a NUL so no id content can forge a collision. - The content allowlist called todo_update, the subagent_* cards and done "invisible chrome". They render their own transcript rows, so a turn-keyed run resuming after one would have run on from the previous message with no break. They are now content; delegation_state and transcript_retraction are not, since neither draws a row. - A user message now clears the open drafts, so a recurring key cannot glue text across a user turn and file it at the pre-user-message position. The paragraph break between two text blocks of one message is still lost, since no runtime marks content blocks. Measured over the same 60 transcripts that costs ~0 real breaks — the model's own deltas already carry the newlines — against 300 joins that would otherwise split mid-word, so verbatim stays the safe side. Noted in the module with the per-block itemId that would remove the trade-off. Also from review: collapse the paired contiguity state into one openStreamKey, return identity as data instead of re-parsing the key prefix, drop the boolean parameter that switched appendFragment's meaning, import the shared AgentChatTranscriptEntry instead of redeclaring it, and keep test timestamps valid past the hundredth envelope. A literal NUL had been written into the source rather than an escape, which made git treat the module as binary and left it with no reviewable diff. It is now written as an escape, with a test pinning the separator. Verified: 5796 identified streams across 60 production transcripts rebuild verbatim, zero mismatches; 39 chat test files, 1133 tests pass. Co-Authored-By: Claude <noreply@anthropic.com>
The user-message case asserted the same role ordering as the recurring-key test while covering strictly less, so the stronger one stands alone. Co-Authored-By: Claude <noreply@anthropic.com>
Keying entries on the messageId/itemId pair split Codex messages into separate
transcript entries, but iOS collapses a text event onto its messageId
(workAssistantMessageStableId) — so on history load it keyed both entries the
same and concatenated them, gluing two messages into one paragraph.
Identity is now read at two granularities. `key` groups fragments into an entry
and matches what clients key bubbles on: messageId when present, else the turn.
`block` — the messageId/itemId pair — decides only whether text continues
verbatim. While the pair holds, fragments are one message's deltas and join with
no separator whatever interleaves; when the provider advances its finer id, the
entry takes a paragraph break. The block is tracked per entry so a concurrent
message's fragments landing in between cannot fake a boundary.
Measured against main over the last 60 production transcripts: entry grouping is
identical (5591 vs 5594 entries, and all three extra entries are the intended
clear-on-user-message hardening — exactly three turn-keyed streams recur across
a user turn in one session), while mid-word splices go 1126 to 0. The three
flagged by the detector are real message boundaries whose next word starts
lowercase ("- ACTIVE\n\nclean").
Because the canonical text now takes that paragraph break where the live stream
does not, the two renditions of a multi-item message differ by exactly that
whitespace and neither is a delta of the other. mergeWorkAssistantText gained
the matching rule: when a merge falls through to blind concatenation and the two
sides are the same text once whitespace is ignored, keep the longer one in
either direction. Without it iOS rendered such messages twice — the same failure
this lane set out to fix, reintroduced from the other side.
Verified: desktop chat suite 40 files / 1133 tests, iOS target 894 tests, both
green; typecheck and lint clean.
Co-Authored-By: Claude <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughChangesTranscript assembly and rendering
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
arul28
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/main/services/chat/chatTranscriptEntries.test.ts (2)
227-231: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLCG multiply overflows float64 precision, degrading split variety.
seed * 1103515245reaches ~2^61 for a 31-bit seed, so the low bits are lost to float rounding before& 0x7ffffffftruncates. The generator stays deterministic, but the split widths it produces are far less varied than intended — which is the entire point of the 200 trials.Math.imulkeeps exact 32-bit semantics.♻️ Proposed fix
const nextInt = (bound: number): number => { - seed = (seed * 1103515245 + 12345) & 0x7fffffff; + seed = (Math.imul(seed, 1103515245) + 12345) & 0x7fffffff; return seed % bound; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/chat/chatTranscriptEntries.test.ts` around lines 227 - 231, Update the nextInt linear-congruential generator to use Math.imul for the seed multiplication, preserving exact 32-bit integer semantics before applying the existing mask and modulo operation. Keep the deterministic seed, constants, and bound behavior unchanged.
213-252: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNo coverage for the
TranscriptEntriesOptionscallbacks.
agentChatService.ts(lines 8357-8364) relies ononEntrySourceOffsetfiring exactly once per emitted entry, in order, soconvertedEntryOffsetsstays index-aligned withconverted. That contract is subtle — the callback fires inside the finalflatMapand is skipped for entries dropped as empty — and nothing here exercises it. A test covering a stream with a dropped empty entry plus a flushed anonymous draft would lock the alignment down.Want me to draft it?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/chat/chatTranscriptEntries.test.ts` around lines 213 - 252, Extend the transcript entries tests to cover the TranscriptEntriesOptions onEntrySourceOffset callback contract: use a stream containing a dropped empty entry and a flushed anonymous draft, then assert the callback fires exactly once for each emitted entry, in emitted order, excluding the dropped entry. Anchor the test around transcriptEntriesFromEnvelopes and verify the reported offsets remain aligned with the resulting entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/desktop/src/main/services/chat/chatTranscriptEntries.test.ts`:
- Around line 227-231: Update the nextInt linear-congruential generator to use
Math.imul for the seed multiplication, preserving exact 32-bit integer semantics
before applying the existing mask and modulo operation. Keep the deterministic
seed, constants, and bound behavior unchanged.
- Around line 213-252: Extend the transcript entries tests to cover the
TranscriptEntriesOptions onEntrySourceOffset callback contract: use a stream
containing a dropped empty entry and a flushed anonymous draft, then assert the
callback fires exactly once for each emitted entry, in emitted order, excluding
the dropped entry. Anchor the test around transcriptEntriesFromEnvelopes and
verify the reported offsets remain aligned with the resulting entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e3c665f-dd7b-42c3-a316-4616d69b596a
⛔ Files ignored due to path filters (3)
docs/features/chat/README.mdis excluded by!docs/**docs/features/chat/transcript-and-turns.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**
📒 Files selected for processing (5)
apps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/chat/chatTranscriptEntries.test.tsapps/desktop/src/main/services/chat/chatTranscriptEntries.tsapps/ios/ADE/Views/Work/WorkErrorAndMessageHelpers.swiftapps/ios/ADETests/ADETests.swift
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile flagged that testing containment anywhere lets a live delta which repeats a long phrase from earlier in the same message look like a second rendition of the whole thing, so it would be discarded. Two renditions of one message always share a beginning, whereas a repeated phrase matches somewhere in the middle, so the check is now a prefix test in either direction. That keeps the canonical-versus-live case it exists for — including a canonical row reconciled against a partial live accumulation — while a mid-message repeat falls through and appends. Worth recording for anyone reading the thread: the discard Greptile describes is real but predates this PR. mergeWorkStreamingReplayText already drops a delta that replays 24 or more characters of earlier text, which is what a test written against that scenario actually proves. This change narrows the newly added path rather than introducing the behavior. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review Addressed the Greptile P1 on Change: the whitespace-insensitive rendition check is now anchored at the start of both texts ( One correction for the record. I tried to pin the reported behavior with a test and it failed — revealing the discard is real but predates this PR. Verified: the five |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83b82cff32
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "subagent_progress", | ||
| "subagent_result", | ||
| "subagent_started", |
There was a problem hiding this comment.
When turn-keyed text lacks both messageId and itemId, a canonical subagent.started, subagent.progress, or subagent.completed event between fragments leaves openStreamKey unchanged, so the fragments are concatenated as one uninterrupted message. These canonical dotted events are normalized into visible subagent cards by appendCollapsedChatTranscriptEvent in chatTranscriptRows.ts, meaning the live renderer keeps the surrounding text runs separate while chat.getTranscript now joins them; this can recreate a canonical/live rendition mismatch for providers that omit message identity. Add the dotted lifecycle variants to this content-boundary allowlist alongside the legacy subagent_* variants.
Useful? React with 👍 / 👎.
Greptile flagged twice that the whitespace-insensitive rendition check discards
legitimate live deltas — first for containment anywhere, then, after narrowing
it to a shared opening, for a delta repeating the message's opening. Both are
right, and a third narrowing would have been the wrong answer: the check only
existed to paper over a divergence this branch introduced.
Canonical text had started taking a paragraph break where the provider advances
its finer id inside one messageId. No renderer does that. Desktop's
shouldMergeTextRows compares messageId, ignores itemId, and joins rows with a
bare `${previous}${next}`; iOS collapses a text event onto its messageId. So
canonical became the only surface that separated there, the two renditions
stopped being deltas of each other, and iOS concatenated them — the same
duplicate render this branch set out to fix.
Identified fragments now concatenate verbatim, exactly as the renderer joins
them, and the invariant is stated as the stronger property it always should have
been: canonical text is byte-identical to what a renderer draws from the same
envelopes. Verified across 4838 messages in the last 60 production transcripts —
zero mismatches against a renderer-faithful reconstruction.
That removes the reason for the client guard, so mergeWorkAssistantText loses
the whitespace branch and both its helpers, and the tests that constrained them
go with it. A heuristic deleted rather than a fourth one added.
Also from review (Codex P2): the canonical dotted subagent lifecycle events —
subagent.started, subagent.progress, subagent.completed — render the same
visible cards as the underscore variants and now sit alongside them in the
content allowlist, so a turn-keyed run split by one keeps its paragraph break.
Verified: desktop chat 39 files / 1132 tests, iOS target 893 tests, typecheck
and lint clean.
Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review Both Greptile P1s and the Codex P2 are addressed — but the first two by removing the code they pointed at rather than narrowing it a third time. The check they flagged only existed to compensate for a divergence this branch had introduced. Canonical text had started taking a paragraph break where the provider advances its finer id inside one Fixed at the source: identified fragments concatenate verbatim, matching the renderer. The invariant is now stated as the stronger property — canonical text is byte-identical to what a renderer draws from the same envelopes — and checked against 4,838 messages across 60 production transcripts, zero mismatches. Consequently Codex P2 applied — Verified: desktop chat 39 files / 1,132 tests · iOS target 893 tests · typecheck + lint clean. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84ded4dfc0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const key = messageId ? `message:${messageId}` : turnId ? `turn:${turnId}` : null; | ||
| if (!key) return null; | ||
| return { key, identified: Boolean(messageId || itemId) }; |
There was a problem hiding this comment.
When a provider omits messageId but supplies itemId, two different items in the same turn receive the same turn: key while identified remains true. If rendered content occurs between them, contiguous is therefore forced true and canonical text concatenates the separate messages without a boundary. Desktop's turnAndItemMatch rejects differing item IDs, and iOS also uses itemId as its fallback stable identity, so providers using this supported fallback can again receive canonical text that disagrees with both renderers.
Useful? React with 👍 / 👎.
| * live emission granularity. It intentionally defaults the other way (unknown | ||
| * types flush) because a wrong guess there only costs batching latency. | ||
| */ | ||
| const TRANSCRIPT_CONTENT_EVENT_TYPES: ReadonlySet<string> = new Set<AgentChatEvent["type"]>([ |
There was a problem hiding this comment.
The allowlist omits known events that create transcript rows, including turn_diagnostics, turn_recovery, codex_turn_recovery, and turn_health (see appendCollapsedChatTranscriptEvent in chatTranscriptRows.ts). For turn-keyed text without message/item identity on either side of one of these events, isTranscriptContentEvent returns false, leaves openStreamKey active, and concatenates the text as one uninterrupted run even though the renderer inserts a row and separates the text. This breaks the canonical/rendered reconciliation invariant for those transcripts.
Useful? React with 👍 / 👎.
| let merged = mergeWorkStreamingText(existing, incoming) | ||
| guard !existing.isEmpty, | ||
| !incoming.isEmpty, | ||
| merged == existing + incoming | ||
| else { return merged } | ||
| guard !incomingIsLiveFragment else { return merged } |
There was a problem hiding this comment.
When a sequenced assistant delta repeats an earlier opening or sufficiently long substring, mergeWorkStreamingText applies its prefix and replay-collapse heuristics before incomingIsLiveFragment is checked. Those branches return transformed text rather than existing + incoming, so the guard never preserves the live delta, causing legitimately repeated output to disappear or the accumulated iOS message to be truncated.
Artifacts
Repro: faithful executable port of the targeted Swift merge branches
- Evidence file captured while the check ran.
Repro: verbose trace showing both live deltas discarded and native tool availability
- The full command output behind this check.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/ios/ADE/Views/Work/WorkErrorAndMessageHelpers.swift
Line: 337-342
Comment:
**Live replay heuristics discard deltas**
When a sequenced assistant delta repeats an earlier opening or sufficiently long substring, `mergeWorkStreamingText` applies its prefix and replay-collapse heuristics before `incomingIsLiveFragment` is checked. Those branches return transformed text rather than `existing + incoming`, so the guard never preserves the live delta, causing legitimately repeated output to disappear or the accumulated iOS message to be truncated.
How can I resolve this? If you propose a fix, please make it concise.
Two Codex P2s, both real violations of the invariant this branch just committed to. A provider that omits messageId but supplies itemId was landing every item in a turn under one `turn:` key, with `identified` true, so rendered content between two items could not break them apart and canonical concatenated two separate messages. Both renderers disagree: iOS keys the bubble on itemId when messageId is absent (workAssistantMessageStableId) and desktop's turnAndItemMatch refuses to merge rows whose item ids differ. Item-only streams now key on itemId, which is exactly what those clients do. The content allowlist was also missing turn_diagnostics, turn_health, turn_recovery, and codex_turn_recovery. All four append a visible row in appendCollapsedChatTranscriptEvent, so turn-keyed text on either side of one has to keep its paragraph break. Re-verified against the last 60 production transcripts, now reconstructing the renderer identity including the itemId fallback: 5912 messages, zero mismatches. Desktop chat 39 files / 1133 tests; no Swift touched this round. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review Both P2s applied — each was a real violation of the invariant this PR commits to. Item-only streams now key on Allowlist gained Re-verified with the renderer reconstruction extended to the @greptile-apps on the P1 at You're describing real behavior: That function is one of seven repair heuristics iOS accumulated to compensate for the host bug this PR fixes. Several are now provably dead, and CI is green at 34/34. |
|
The scoping argument holds. What this PR adds on the iOS side is the Accepted. The one thing worth doing before merge: make sure |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0319cc39d8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard !incomingIsLiveFragment else { return merged } | ||
| return incoming.count >= existing.count ? incoming : existing |
There was a problem hiding this comment.
When one identified assistant message spans two byte-cursor transcript pages, both page entries have sequence == nil but neither is a whole message: getChatTranscriptPage can stop at an envelope boundary and iOS later prepends the older chunk. This branch therefore keeps only the longer chunk instead of concatenating the disjoint older and newer chunks, silently dropping part of the assistant response when canonical paging is used (for example with a host lacking chat.getChatEventHistoryPage or when that action fails). The merge needs to distinguish separate canonical page chunks from overlapping whole-message renditions rather than treating every sequence-less envelope as complete.
Useful? React with 👍 / 👎.
| * live emission granularity. It intentionally defaults the other way (unknown | ||
| * types flush) because a wrong guess there only costs batching latency. | ||
| */ | ||
| const TRANSCRIPT_CONTENT_EVENT_TYPES: ReadonlySet<string> = new Set<AgentChatEvent["type"]>([ |
There was a problem hiding this comment.
For existing transcripts containing turn-keyed text around a context_compact event, this allowlist now treats the compaction event as invisible chrome because neither context_compact nor codex_context_compaction is included. Desktop's appendCollapsedChatTranscriptEvent instead routes those events through isContextCompactionChatEvent and appends a visible row, so openStreamKey must be cleared; otherwise the canonical reconstruction concatenates the text on both sides verbatim rather than preserving the separate rendered messages. This regresses the prior behavior, where every non-text event ended the run.
Useful? React with 👍 / 👎.
| let merged = mergeWorkStreamingText(existing, incoming) | ||
| guard !existing.isEmpty, | ||
| !incoming.isEmpty, | ||
| merged == existing + incoming | ||
| else { return merged } | ||
| guard !incomingIsLiveFragment else { return merged } |
There was a problem hiding this comment.
When a sequenced live assistant delta repeats an earlier prefix, suffix, or sufficiently long substring of the accumulated message, mergeWorkStreamingText transforms or discards it before incomingIsLiveFragment is checked, causing legitimate repeated assistant output to remain missing on iOS.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/ios/ADE/Views/Work/WorkErrorAndMessageHelpers.swift
Line: 337-342
Comment:
**Live replay heuristics discard deltas**
When a sequenced live assistant delta repeats an earlier prefix, suffix, or sufficiently long substring of the accumulated message, `mergeWorkStreamingText` transforms or discards it before `incomingIsLiveFragment` is checked, causing legitimate repeated assistant output to remain missing on iOS.
How can I resolve this? If you propose a fix, please make it concise.Codex found that it loses data. When one assistant message spans two byte-cursor transcript pages, both chunks arrive with sequence == nil and neither is whole, so treating every sequence-less envelope as a complete message kept the longer chunk and silently dropped the rest of the answer. The guard existed to reconcile canonical text against the live stream when the two disagreed. They no longer disagree: canonical text is byte-identical to what a renderer draws from the same envelopes, verified across 5912 messages. So iOS merges every assistant fragment through mergeWorkStreamingText again, exactly as it did before this branch, and the tests that only constrained the guard go with it. testCanonicalAndLiveRowsForOneMessageRenderItOnce still passes without it, which is the point: the host fix alone is what stops the message rendering twice. That also answers the Greptile P1 about replay heuristics running ahead of the live-fragment check — it described mergeWorkStreamingText, which this branch no longer wraps or modifies. Retiring those heuristics stays scoped to its own lane. Also from review (Codex P2): context_compact and codex_context_compaction append a visible row through isContextCompactionChatEvent, so they belong in the content allowlist; turn-keyed text on either side of a compaction keeps its break. Verified: desktop chat 39 files / 1133 tests, typecheck clean. iOS 890 tests with one pre-existing flake in unrelated lane-deletion ordering, which passes 3/3 in isolation. The ade-cli CI failure on the previous commit was likewise a flake — remoteLauncher asserts a spawn count under an 80ms wall-clock budget — and the job passed on re-run. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review P1 (paged canonical chunks) — fixed by deleting the guard entirely. You're right that it loses data: two chunks of one message from Rather than teach it to distinguish page chunks from whole messages, I removed it. It only ever existed to reconcile canonical text against the live stream when the two disagreed, and after the previous commit they don't — canonical is byte-identical to what a renderer draws, checked across 5,912 messages. iOS merges through The evidence that this is right: P2 (compaction events) — applied. @greptile-apps this also resolves your P1 at Two flakes seen, neither related to this diff:
Net for this PR the iOS side now has less code than it started with. |
|
The guard is gone — The reasoning is sound:
P2 (compaction events in the allowlist) is straightforward and correct. The approach is right. Good to go from my side. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fd778e759
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| | `apps/desktop/src/main/services/chat/projectSlashCommandDiscovery.ts` | Unified project-wide slash command discovery. Merges commands from Claude, Codex, and Cursor discovery modules into a single deduplicated list, filtering `/login`. Used by the ADE Code TUI's `discoverProjectSlashCommands` so all providers see the same cross-provider command catalog. | | ||
| | `apps/desktop/src/main/services/chat/slashCommandPromptExpansion.ts` | Provider-routed slash command prompt expansion. Given a provider, cwd, and slash command input, resolves the command's markdown body into prompt text through the appropriate provider-specific resolution path (Claude, Codex, or Cursor). Built-in and runtime-registered commands are skipped so the SDK handles them natively. Consumed by `agentChatService` to expand slash commands before dispatch. | | ||
| | `apps/desktop/src/main/services/chat/chatTextBatching.ts` | Batches streaming assistant text fragments (100 ms) before emission to reduce renderer re-renders. | | ||
| | `apps/desktop/src/main/services/chat/chatTranscriptEntries.ts` | Flattens an envelope stream into the canonical role-tagged `AgentChatTranscriptEntry[]` behind `chat.getTranscript`, the cursor-paged chat-history reader, and the internal auto-title/handoff transcript reads. Its invariant is that a message's canonical text is the **verbatim** concatenation of its `text` fragments: fragments sharing provider identity (the `messageId`/`itemId` *pair* — Claude sets only `messageId`, Codex sets `messageId` per turn and `itemId` per message, so either alone merges distinct messages) concatenate untouched no matter what interleaves, and only `isTranscriptContentEvent` rows (a deliberate *allowlist* of rendered types) end a run, and only for fragments ADE cannot tie to a provider message. Whitespace-only fragments are real deltas and are dropped only when no run is open to continue. Inventing a `"\n\n"` here spliced separators mid-word and made clients that reconcile canonical text against the live stream (iOS, web) render a message twice. See [Transcript and turns](transcript-and-turns.md#canonical-assistant-text-fragile--read-before-editing). | |
There was a problem hiding this comment.
This entry says continuation is keyed by the messageId/itemId pair and that either ID alone merges distinct messages, but assistantStream actually selects messageId alone whenever present, and the new tests intentionally concatenate fragments whose itemIds differ under one messageId. The more detailed transcript guide also documents message-level grouping. Because this README is presented as the invariant to preserve, it could lead a future maintainer to reintroduce pair-based grouping and the duplicate-render regression; describe the actual messageId, then itemId, then turn fallback policy instead.
Useful? React with 👍 / 👎.
| timestamp: "2026-07-26T01:00:01.000Z", | ||
| sequence: 42, | ||
| event: .assistantText(text: "ifier chain needed:", turnId: "turn-1", itemId: "msg-a") | ||
| ), | ||
| ] | ||
|
|
There was a problem hiding this comment.
When a sequenced live assistant delta legitimately repeats an earlier prefix or sufficiently long substring of the accumulated message, the existing replay heuristics still transform or discard that delta because the new test covers only canonical-plus-suffix reconciliation, causing repeated provider output to remain missing from the iOS message.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/ios/ADETests/ADETests.swift
Line: 12963-12968
Comment:
**Live replay still drops deltas**
When a sequenced live assistant delta legitimately repeats an earlier prefix or sufficiently long substring of the accumulated message, the existing replay heuristics still transform or discard that delta because the new test covers only canonical-plus-suffix reconciliation, causing repeated provider output to remain missing from the iOS message.
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Reported as a mobile rendering bug: assistant text in an ADE Work chat was cut off mid-word and then repeated in the next bubble. Mobile is only where it was visible.
Root cause
chat.getTranscriptrebuilds an assistant message from its streamedtextfragments. It treated two fragments as contiguous only when the immediately preceding envelope was the same message, and glued them with"\n\n"otherwise. Ephemeral chrome that no renderer draws —activityhints, subagent progress, token usage — lands between deltas constantly, so the separator landed inside words. Straight from the reported session's transcript:→ canonical text became
'…no new mod\n\nifier chain needed:'.Desktop renders the envelope stream directly and concatenates adjacent text rows, skipping invisible events, so it never saw the corrupted string. iOS holds both the live fragments and this canonical text and reconciles them; because the two no longer matched, its overlap heuristics fell through to
existing + incomingand printed the message twice. Every previous attempt at this bug went into those client heuristics, which is why it kept coming back.A second defect surfaced while testing: whitespace-only fragments were dropped outright, so
"chain" + " " + "needed"rebuilt as"chainneeded", and markdown hard breaks (" \n") vanished.Measured over the last 60 production transcripts: 1,126 mid-word splices and 4,181 dropped whitespace fragments.
The fix
Identity is now read at two granularities, and that split is the whole design:
keygroups fragments into an entry —messageIdwhen present, else the turn. It deliberately matches the identity clients key bubbles on (iOS collapses a text event onto itsmessageId). Grouping more finely than a client does makes the client silently concatenate two entries.blockdecides verbatim continuation — themessageId/itemIdpair, tracked per entry. Runtimes disagree about which field is finer-grained (Claude sets onlymessageId; Codex setsmessageIdto a per-turn UUID anditemIdto the provider message id), so neither alone can be believed. While the pair holds, fragments concatenate verbatim whatever interleaves; when the provider advances its finer id, the entry takes a paragraph break.isTranscriptContentEventis an allowlist of content types on purpose: a future event type defaults to "does not break the run", which at worst drops a paragraph break, whereas the inverse default corrupts words.On the client,
mergeWorkAssistantTextholds the matching rule — a whole-message rendition may replace the accumulation or be dropped, but is never concatenated onto it, and two renditions differing only in whitespace never duplicate.The logic moved out of
agentChatService.tsintochatTranscriptEntries.tsso it is unit-testable.Verification
main(5591 → 5594; all three extra entries are the intended clear-on-user-message hardening, matching exactly three recurring turn-keyed streams)"· ACTIVE\n\nclean")No data migration: the stored JSONL fragments were never corrupted, only the rebuild, so the fix applies retroactively to every existing transcript on read.
Follow-ups (deliberately not in this PR)
itemIdon Claude text events (the thinking emitter already does) — removes the last case where two text blocks in one message lose their paragraph breakworkStreamingTextByCollapsingRepeatedTailReplayfirst — it deletes legitimately repeated text on every fragmentonEntrySourceOffsetbuilds a parallel array whose alignment is contractual but untyped🤖 Generated with Claude Code
Summary by CodeRabbit
Greptile Summary
The PR changes canonical transcript reconstruction and adds focused regression coverage.
Confidence Score: 4/5
The PR is not yet safe to merge because iOS can still discard or replace legitimately repeated sequenced live deltas.
The host-side reconstruction fixes canonical/live disagreement, but iOS continues applying canonical-replay heuristics to live provider fragments, leaving the previously reported repeated-output loss reachable.
Files Needing Attention: apps/ios/ADETests/ADETests.swift and apps/ios/ADE/Views/Work/WorkErrorAndMessageHelpers.swift
What T-Rex did
Important Files Changed
Sequence Diagram
Prompt To Fix All With AI
Reviews (5): Last reviewed commit: "Delete the client-side rendition guard" | Re-trigger Greptile