fix(chat): scope a comparison pane's attachments to its own lane - #363
Merged
Conversation
A pane filters `content` per lane but carried `attachments` through whole, so every surface a bubble draws from the attachment list rather than from a pane-filtered part rendered once per pane.
Contributor
Android debug APKArtifact:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A Compare Models turn rendered the message's attachment-driven surfaces twice, once per pane. Each pane filters
contentper lane but carriedattachmentsthrough whole, so anything a bubble draws from the attachment list rather than from a pane-filtered part was emitted in both panes. Panes now see only the attachments their own lane produced.Three duplications this removes:
set_memoryin one lane rendered its own card in that pane and — because its tool call is absent from the other pane's content — read as an orphan there and got a second card.message.attachmentswith no tool-call join at all, and duplicated unconditionally.Closes #359.
Changes
attachmentsForPane(util/ComparisonDisplay.kt) — an attachment follows the lane whose parts contain the call that produced it; one attributable to no call in the turn renders once, in the primary pane.buildComparisonDisplayMessages' parallel branch and intocollapseParallelToPrimary. In the single list this is a removal, not only a de-duplication: the added agent's attachments are dropped alongside its content, so a secondary-lane office preview or memory write that previously rendered there now renders nowhere. That path also feeds in-conversation search and the iOS screen.renderedToolCallIdsgains a KDoc note on why it stops one level down and is not interchangeable with the recursive walk.Testing
./gradlew test detektMetadataCommonMain detekt :app:lint :app:assembleDebug— green.ComparisonDisplayTestcases. 6 of the 7 fail against the pre-change file; the seventh is the non-parallel regression guard and passes either way.Notes
MemoryArtifactsevery attachment once above both columns, with notoolCallIdfilter (ParallelContent.tsx:275,MemoryArtifacts.tsx). Placing an unattributable write in the primary pane matches that. Routing an attributable one to its own lane is a deliberate divergence, needed because mobile also renders an inline per-call card that upstream has no equivalent of.ComparisonDisplayrather than at the render site because the panes are derived in a pure function, so the fix is assertable in JVM tests —feature:chathas only an on-device Compose harness, the same reasonsendButtonModeForis a pure transform pulled out of its composable. A render-site fix would have to be repeated on every bubble surface that readsmessage.attachments.outputToolCallIds(recursive) and not the similarly-namedrenderedToolCallIds, which stops one level down because that is where the subagent trace recursion stops — a call two levels down is drawn by nothing.outputToolCallIdsnow serves two purposes across three call sites: attachment hoisting (activity groups and subagent cards) and this lane attribution.memory.agent.enabledis not configured on the test server), so the rendering path is covered but not the server behaviour that produces it.