Chat Tool Streaming#174
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughUpdates iOS work chat to display visual stack hints instead of text previews in collapsed tool-call clusters. Adds stable timeline item ID logic that prefers Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/ios/ADE/Views/Work/WorkChatRichCardViews.swift (1)
197-217: Minor: drop the explicitelse { EmptyView() }.SwiftUI's
@ViewBuilderalready treats a missingelsebranch as empty, so the explicitEmptyView()branch is noise. The rest of the ZStack/backdrop layout withallowsHitTesting(false)and the compensating.padding(.bottom, 6)reads well.♻️ Optional cleanup
ZStack(alignment: .bottom) { if !effectiveExpanded && group.count > 1 { collapsedStackBackdrop - } else { - EmptyView() } VStack(alignment: .leading, spacing: 10) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Work/WorkChatRichCardViews.swift` around lines 197 - 217, Remove the redundant else branch returning EmptyView() inside the ZStack: locate the conditional that checks `if !effectiveExpanded && group.count > 1` (used with `collapsedStackBackdrop`) and delete the `else { EmptyView() }` branch so the `@ViewBuilder` implicitly treats the missing else as empty; keep the surrounding ZStack, `collapsedStackBackdrop`, `effectiveExpanded`, and the trailing `.padding(.bottom, !effectiveExpanded && group.count > 1 ? 6 : 0)` unchanged.apps/ios/ADE/Views/Work/WorkTranscriptParser.swift (1)
32-34: Consider reusingworkStableTimelineItemIdfor consistency.This inline logic duplicates the
workStableTimelineItemIdhelper inapps/ios/ADE/Views/Work/WorkEventMapping.swift(lines 5-8). SinceitemIdhere is optional while the helper takesString, a small wrapper keeps the two call sites in lockstep and avoids drift if the preference rule changes later:♻️ Proposed refactor
let itemId = eventDict["itemId"] as? String - let logicalItemId = (eventDict["logicalItemId"] as? String)? - .trimmingCharacters(in: .whitespacesAndNewlines) - let stableToolItemId = (logicalItemId?.isEmpty == false ? logicalItemId : nil) ?? itemId + let logicalItemId = (eventDict["logicalItemId"] as? String)? + .trimmingCharacters(in: .whitespacesAndNewlines) + let stableToolItemId: String? = { + if let logical = logicalItemId, !logical.isEmpty { return logical } + return itemId + }()Or, if you expose an optional overload of the helper, route both files through it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Work/WorkTranscriptParser.swift` around lines 32 - 34, The logic computing stableToolItemId duplicates workStableTimelineItemId; replace it by calling that helper to keep behavior consistent: wrap or add an optional overload around workStableTimelineItemId so you can pass itemId (which is Optional<String>) from WorkTranscriptParser (where logicalItemId and itemId are available), and then use the helper to compute stableToolItemId instead of inlining the trimming/isEmpty logic; update the callsite in WorkTranscriptParser to call the new wrapper/overload and remove the duplicated logic around logicalItemId and itemId.apps/ios/ADETests/ADETests.swift (1)
5689-5706: Tighten the payload-preservation assertions.These tests prove dedupe/status, but
XCTAssertNotNilwould still pass if args/result text were wrong or empty. Please assert the expected payload content on both direct event and parser paths.🧪 Proposed assertion tightening
XCTAssertEqual(cards.count, 1) XCTAssertEqual(cards.first?.id, "tool-logical-1") XCTAssertEqual(cards.first?.status, .completed) - XCTAssertNotNil(cards.first?.argsText) - XCTAssertNotNil(cards.first?.resultText) + XCTAssertTrue(cards.first?.argsText?.contains("pwd") == true) + XCTAssertTrue(cards.first?.resultText?.contains("/tmp/project") == true) @@ XCTAssertEqual(cards.count, 1) XCTAssertEqual(cards.first?.id, "tool-logical-1") XCTAssertEqual(cards.first?.status, .completed) + XCTAssertTrue(cards.first?.argsText?.contains("pwd") == true) + XCTAssertTrue(cards.first?.resultText?.contains("/tmp/project") == true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADETests/ADETests.swift` around lines 5689 - 5706, Replace the loose XCTAssertNotNil assertions with exact-content checks: assert that cards.first?.argsText equals the expected args string (e.g., the JSON or "cmd":"pwd" payload) and that cards.first?.resultText equals the expected result string (e.g., "/tmp/project"), and also add assertions against the parsed transcript (the output of parseWorkChatTranscript(raw)) to verify the corresponding tool_call and tool_result event payloads contain those exact args/result values; update testParseWorkChatTranscriptUsesLogicalItemIdForStableToolCards to check both the parsed transcript entries and the built WorkToolCard fields (using parseWorkChatTranscript, buildWorkToolCards, raw, transcript, and cards identifiers).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/ios/ADE/Views/Work/WorkChatRichCardViews.swift`:
- Around line 197-217: Remove the redundant else branch returning EmptyView()
inside the ZStack: locate the conditional that checks `if !effectiveExpanded &&
group.count > 1` (used with `collapsedStackBackdrop`) and delete the `else {
EmptyView() }` branch so the `@ViewBuilder` implicitly treats the missing else as
empty; keep the surrounding ZStack, `collapsedStackBackdrop`,
`effectiveExpanded`, and the trailing `.padding(.bottom, !effectiveExpanded &&
group.count > 1 ? 6 : 0)` unchanged.
In `@apps/ios/ADE/Views/Work/WorkTranscriptParser.swift`:
- Around line 32-34: The logic computing stableToolItemId duplicates
workStableTimelineItemId; replace it by calling that helper to keep behavior
consistent: wrap or add an optional overload around workStableTimelineItemId so
you can pass itemId (which is Optional<String>) from WorkTranscriptParser (where
logicalItemId and itemId are available), and then use the helper to compute
stableToolItemId instead of inlining the trimming/isEmpty logic; update the
callsite in WorkTranscriptParser to call the new wrapper/overload and remove the
duplicated logic around logicalItemId and itemId.
In `@apps/ios/ADETests/ADETests.swift`:
- Around line 5689-5706: Replace the loose XCTAssertNotNil assertions with
exact-content checks: assert that cards.first?.argsText equals the expected args
string (e.g., the JSON or "cmd":"pwd" payload) and that cards.first?.resultText
equals the expected result string (e.g., "/tmp/project"), and also add
assertions against the parsed transcript (the output of
parseWorkChatTranscript(raw)) to verify the corresponding tool_call and
tool_result event payloads contain those exact args/result values; update
testParseWorkChatTranscriptUsesLogicalItemIdForStableToolCards to check both the
parsed transcript entries and the built WorkToolCard fields (using
parseWorkChatTranscript, buildWorkToolCards, raw, transcript, and cards
identifiers).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9ddd8b62-0279-48c4-9639-fb86be834273
📒 Files selected for processing (4)
apps/ios/ADE/Views/Work/WorkChatRichCardViews.swiftapps/ios/ADE/Views/Work/WorkEventMapping.swiftapps/ios/ADE/Views/Work/WorkTranscriptParser.swiftapps/ios/ADETests/ADETests.swift
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
| case .fileChange(let path, let diff, let kind, let itemId, _, let turnId, let status): | ||
| return .fileChange(path: path, diff: diff, kind: kind.rawValue, status: toolStatus(from: status ?? "running"), itemId: itemId, turnId: turnId) | ||
| case .fileChange(let path, let diff, let kind, let itemId, let logicalItemId, let turnId, let status): | ||
| return .fileChange(path: path, diff: diff, kind: kind.rawValue, status: toolStatus(from: status ?? "running"), itemId: workStableTimelineItemId(itemId: itemId, logicalItemId: logicalItemId), turnId: turnId) |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
Normalizing file_change events to logicalItemId turns distinct per-file updates into the same iOS card key. The changed code now does:
// apps/ios/ADE/Views/Work/WorkEventMapping.swift
case .fileChange(let path, let diff, let kind, let itemId, let logicalItemId, let turnId, let status):
return .fileChange(path: path, diff: diff, kind: kind.rawValue, status: toolStatus(from: status ?? "running"), itemId: workStableTimelineItemId(itemId: itemId, logicalItemId: logicalItemId), turnId: turnId)I verified that buildWorkFileChangeCards stores one WorkFileChangeCardModel per itemId (@apps/ios/ADE/Views/Work/WorkTimelineHelpers.swift:376-393), while the desktop emitter can intentionally produce multiple file changes with unique raw IDs but one shared logical ID (itemId: ${part.id}:${file}, logicalItemId: part.id in @apps/desktop/src/main/services/chat/agentChatService.ts:7629-7636). After this change, those files all collapse onto the same iOS itemId, so earlier paths are overwritten and only the last changed file remains visible in both live mapping and transcript parsing. Keep raw itemId for file_change until the iOS file-change card builder can merge multiple paths under one logical item.
- File-change events deliberately keep raw itemId: OpenCode's patch emitter produces one event per file with a shared logicalItemId but distinct raw IDs. Collapsing to logicalItemId was dropping all but the last file. - Restore streaming progress by auto-expanding running tool cards inside groups (localExpanded || .running), same as before the timeline cleanup. - Route WorkTranscriptParser through a new optional-itemId overload of workStableTimelineItemId so both paths share the resolution policy. - Drop redundant EmptyView else branch in the collapsed-stack ZStack. - Bump collapsed-stack bottom padding to 10pt so the y:8 backdrop offset stays inside the layout on smaller devices. - Add regression tests covering distinct files under a shared logicalItemId through both the direct-event and transcript-parser paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
New Features
Improvements
Tests
Greptile Summary
This PR introduces two related improvements: (1) a
logicalItemIdfield is now used as a stable key for tool/command/webSearch cards so that streaming updates (call → result) collapse onto the same card rather than creating duplicates, and (2) the collapsed group view replaces a text-based mini-preview with a purely visual stacked-card backdrop effect.The deduplication logic is well-factored into
workStableTimelineItemIdoverloads with appropriate test coverage, and the intentionalfileChangecarve-out (keeping rawitemIdto preserve per-file identity under a sharedlogicalItemId) is documented and regression-tested.Confidence Score: 5/5
Safe to merge — changes are well-scoped, covered by regression tests, and prior review concerns have been addressed.
All findings from previous review threads are addressed (auto-expand preserved for running cards, backdrop padding added, shared-function comment noted). No new P0/P1 issues found. Good test coverage across both code paths (event mapping and transcript parser).
No files require special attention.
Important Files Changed
workStableTimelineItemIdoverloads and wires them into toolCall, toolResult, webSearch, and command event mappings; fileChange intentionally keeps raw itemId.logicalItemIdfrom the JSON envelope and appliesworkStableTimelineItemIdfor tool/command/webSearch events; file_change is correctly left on raw itemId.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[AgentChatEvent / JSON transcript] --> B{Event type} B -->|toolCall / toolResult\nwebSearch / command| C[workStableTimelineItemId\nitemId + logicalItemId] B -->|fileChange| D[Keep raw itemId\nper-file identity preserved] C --> E{logicalItemId\nnon-empty?} E -->|Yes| F[Use logicalItemId\nas stable card key] E -->|No| G[Fall back to itemId\nor workFallbackItemID] F --> H[buildWorkToolCards\nbuildWorkCommandCards\nbuildWorkTimelineSnapshot] G --> H D --> I[buildWorkFileChangeCards\none card per file] H --> J[WorkToolGroupCardView\nCollapsed: stack backdrop\nExpanded: full member rows] I --> JReviews (2): Last reviewed commit: "Address PR #174 review: keep file-change..." | Re-trigger Greptile