streaming: retire legacy vocabulary#32462
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82cef30c07
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // legacy rows with `MessageStore` snapshots using stable, deterministic | ||
| // ids, so the projector's input is already unique by id and the | ||
| // previous defensive dedupe pass is no longer needed. | ||
| let visibleMessages = paginatedVisibleMessages |
There was a problem hiding this comment.
Preserve row de-dupe before rendering
When duplicate ChatMessage.ids reach the list, this pass-through lets both rows into state.rows; MessageListContentView then builds Dictionary(uniqueKeysWithValues:) from those IDs, which traps on duplicates. Duplicate IDs are still possible outside the reducer path—for example, pagination/history merging prepends chatMessages + self.messages without ID de-duping, so a retried/late older-page response can leave the same history row in the transcript twice. The previous filter made that case harmless; without an equivalent guard upstream, scrolling/loading history can crash the chat view.
Useful? React with 👍 / 👎.
Summary
Final PR in the streaming-message-architecture series. Scope is intentionally a focused cleanup rather than a full vocabulary deletion:
TranscriptProjectordedupe-by-id removed. TheMessageStreamReduceris the sole writer for assistant content and applies events idempotently per(messageId, blockIndex, seq).renderedMessagesproduces stable, deterministic ids upstream, so the projector's defensive Set-based dedupe pass is no longer needed.MessageStore+ChatMessageBridgedocumentation refreshed to reflect the post-PR-4 architecture and theMessageStreamReduceridempotency invariant that keeps the streaming-then-reload duplication structurally impossible.assistant_text_deltaandmessage_completeupdated. Documents that the main agent loop always stampsmessageId/blockIndex/seq(synthetic emitters for canned greetings / live-voice / wake-target / recording handlers still emit without them and are consumed by channel adapters, not the reducer path). Documents thatmessage_completeremains the canonical post-persistence end-of-turn signal for the wider event-consumer fleet (CLI, voice bridge, channel retry sweep, background dispatch) —message_closeis the pre-persistence streaming signal.Scope notes for reviewers
The full retirement described in the plan — deleting
currentAssistantMessageId,streamingDeltaBuffer,partialOutputBuffer,clearCurrentTurnTracking,appendTextToCurrentMessage, and the legacymessages: [ChatMessage]array onChatViewModel— touches ~160 call sites acrossChatViewModel.swift(2,910 LOC),ChatActionHandler.swift(1,604 LOC),MessageSendCoordinator.swift(901 LOC),ChatViewModel+StreamingHelpers.swift(541 LOC),ChatViewModel+SurfaceHandling.swift(475 LOC), and the test suites. Many of these call sites usecurrentAssistantMessageIdfor load-bearing semantics beyond text streaming — anchoring tool results, partial output buffering, surface widget routing, send-coordination state, history reload reconciliation, and watchdog recovery. Migrating each of those concerns to read fromMessageStore(and replacing the legacymessagesarray consumers in send-coordination, queued-message tracking, and the surface handlers) is a multi-PR effort, not a single deletion.The architectural improvement is already in place from PR 4: the chat list renders from
MessageStorevia the bridge, and the reducer's idempotent apply makes the original duplication symptom structurally impossible. The legacy code paths run in parallel but their output is silently dropped byrenderedMessageswhenever aMessageStoresnapshot exists, so they're inert from the renderer's perspective. Retiring them is straightforward but mechanical follow-up work.Completes the streaming-message-architecture plan (PR 6 of 6).
Part of plan: streaming-message-architecture.md