Repository navigation
Fix iOS chat transcript expansion anchoring - #7057
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughAdds pre-layout transcript anchor handling, a debug-only terminal log preview path, stable accessibility identifiers for expansion controls, and UI tests that verify transcript scroll position stays stable after expansion. ChangesTranscript scroll restoration and preview wiring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 2 warnings)
✅ Passed checks (20 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 |
e852950 to
328d7f7
Compare
![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
🤖 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.
Inline comments:
In
`@Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptTableView.swift`:
- Around line 217-222: Refresh the viewport snapshot before using
oldViewport?.wasAtBottom in ChatTranscriptTableView’s scroll/state restoration
flow. In the branch that calls restoreKeyboardViewport, scrollToBottom, or
restore, make sure the current viewport state is captured from the table view
first so lastViewport reflects the latest scroll position before deciding
whether to snap to bottom or restore the anchor. Use the existing
restoreKeyboardViewport(_:in:), scrollToBottom(in:animated:), and restore(_:in:)
paths as the reference points for where to update the snapshot.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 051e5740-2080-4000-b219-efecda9d5479
📒 Files selected for processing (6)
Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptTableView.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptUITableView.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/Rows/ChatTerminalCardView.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/Rows/ChatToolUseRowView.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/Rows/TerminalCommandBlockView.swiftios/cmuxUITests/cmuxUITests.swift
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if boundsChanged, let oldViewport { | ||
| restoreKeyboardViewport(snapshot: oldViewport, in: tableView) | ||
| } else if isAtBottom.wrappedValue { | ||
| } else if oldViewport?.wasAtBottom == true { |
There was a problem hiding this comment.
scrollToBottom() and lastViewport update
When update() calls scrollToBottom() (because wasAtBottom was true), it calls tableView.setContentOffset(…, animated: false). UIKit marks the table as needing layout (setNeedsLayout) but does not call layoutSubviews synchronously — recordViewport() is therefore not called until the next run-loop pass. If a SwiftUI self-sizing layout fires in between (before that deferred layout), oldViewport passed here still holds the pre-scrollToBottom snapshot with wasAtBottom == false, and the branch falls through to restore(oldAnchor) instead. The anchor captured at the bottom will keep the first-visible row in place, but newly added rows that landed below the viewport remain hidden — the "stay pinned to bottom during live output" invariant is silently broken.
The previous code used isAtBottom.wrappedValue, which is written synchronously by setAtBottom(true) inside scrollToBottom(), so it was always up-to-date for this path. One option is to call recordViewport() (or an equivalent snapshot update on lastViewport) immediately after setContentOffset inside scrollToBottom() so that lastViewport.wasAtBottom is authoritative before control returns to any callers that may trigger further layouts.
328d7f7 to
d83b81e
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptTableView.swift (1)
101-103: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCapture the anchor from the pre-update transcript.
Because Line 140 swaps
itemsbefore reload, the callback from Lines 101-103 reachesitems[indexPath.row]in Lines 227-235 against the new transcript while the visible rows still describe the old table. Any insertion/removal above the viewport will therefore anchor to the wrongidand restore the wrong row. Snapshot the visible row’s id before replacingitems, or derive it from the currently visible cell/model instead of the mutated array. As per path instructions, transcript correctness should not depend on “more than one disagreeing source of truth for the same fact.”Also applies to: 140-147, 227-235
🤖 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 `@Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptTableView.swift` around lines 101 - 103, The transcript anchor lookup is reading from the updated items array instead of the pre-update transcript, so the visible row can map to the wrong id after inserts/removals. Update ChatTranscriptTableView so the anchor is captured before items is swapped, or derive it from the currently visible cell/model rather than items inside firstVisibleAnchor(in:) and the anchorBeforeLayout callback. Make sure the reload path that replaces items preserves the old visible row identity across the update so restoration uses the pre-change transcript state.Source: Path instructions
🤖 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.
Outside diff comments:
In
`@Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptTableView.swift`:
- Around line 101-103: The transcript anchor lookup is reading from the updated
items array instead of the pre-update transcript, so the visible row can map to
the wrong id after inserts/removals. Update ChatTranscriptTableView so the
anchor is captured before items is swapped, or derive it from the currently
visible cell/model rather than items inside firstVisibleAnchor(in:) and the
anchorBeforeLayout callback. Make sure the reload path that replaces items
preserves the old visible row identity across the update so restoration uses the
pre-change transcript state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b1d72a04-e0e1-49c5-ba20-21561addb5b9
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (2)
Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptTableView.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptUITableView.swift
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@ios/cmuxUITests/cmuxUITests.swift`:
- Around line 2569-2572: The tap helper is still using a hard-coded coordinate
offset instead of the toggle’s accessibility identifier, which makes the
cmuxUITests regression brittle. Update the helper around the block tap logic to
target the control via the TerminalCommandBlockToggle-\(block.id) accessibility
identifier rather than computing an offset from block.frame. Use the existing
block.id and the relevant tap helper in cmuxUITests.swift so the test locates
the toggle reliably across layout and device changes.
In
`@Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptTableView.swift`:
- Around line 154-156: The `pendingContentUpdateAnchor` in
`ChatTranscriptTableView` is being retained beyond the reload that created it,
so it can be reused by unrelated later resizes. Update the reload path around
`restore(_:in:)` and the later `contentChanged` handling so the anchor is scoped
to a single reload cycle and cleared once that reload finishes or is superseded;
keep the fix centered on `pendingContentUpdateAnchor` and the restore logic that
consumes it.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swift`:
- Around line 65-71: The DEBUG-only terminal log preview is being exposed
through the production root view via
CMUXMobileRootView.shouldShowTerminalLogPreview and rootContent, which adds a
test/demo seam to shipped Sources code. Remove this conditional bootstrap path
from CMUXMobileRootView and move the preview entrypoint into a test/demo-only
harness or UI-test-specific wrapper, keeping production rootContent free of
UITestConfig-driven debug routing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ef955a57-ea38-47f7-997f-30b73b1277ca
📒 Files selected for processing (6)
Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptTableView.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/Rows/TerminalCommandBlockView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalLogDemoScreen.swiftPackages/iOS/CmuxMobileSupport/Sources/CmuxMobileSupport/UITestConfig.swiftios/cmuxUITests/cmuxUITests.swift
| let blockFrame = block.frame | ||
| app.coordinate(withNormalizedOffset: .zero) | ||
| .withOffset(CGVector(dx: blockFrame.minX + 84, dy: blockFrame.minY + 118)) | ||
| .tap() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the toggle's accessibility identifier instead of a hard-coded tap offset.
This helper ignores the new TerminalCommandBlockToggle-\(block.id) contract and taps blockFrame.minX + 84 / minY + 118 instead. Any row-layout or device-geometry change can miss the control and make the regression test flaky.
🤖 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 `@ios/cmuxUITests/cmuxUITests.swift` around lines 2569 - 2572, The tap helper
is still using a hard-coded coordinate offset instead of the toggle’s
accessibility identifier, which makes the cmuxUITests regression brittle. Update
the helper around the block tap logic to target the control via the
TerminalCommandBlockToggle-\(block.id) accessibility identifier rather than
computing an offset from block.frame. Use the existing block.id and the relevant
tap helper in cmuxUITests.swift so the test locates the toggle reliably across
layout and device changes.
| private var shouldShowTerminalLogPreview: Bool { | ||
| #if os(iOS) && DEBUG | ||
| return UITestConfig.terminalLogPreviewEnabled | ||
| #else | ||
| return false | ||
| #endif | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Don't route a new DEBUG-only fixture through the production root view.
This adds a UI-test/demo-only screen to rootContent via UITestConfig.terminalLogPreviewEnabled, which expands the app's production Sources/ surface with another debug seam. Move this preview entrypoint into a test/demo-only harness instead of wiring it through shipped bootstrap code. As per coding guidelines, **/Sources/**/*.swift: Do not add test-only or debug-only seams in production Swift source files. As per path instructions, **/Sources/**/*.swift: flag added test-only or debug-only seams in production source.
Also applies to: 105-112, 221-222
🤖 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
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swift`
around lines 65 - 71, The DEBUG-only terminal log preview is being exposed
through the production root view via
CMUXMobileRootView.shouldShowTerminalLogPreview and rootContent, which adds a
test/demo seam to shipped Sources code. Remove this conditional bootstrap path
from CMUXMobileRootView and move the preview entrypoint into a test/demo-only
harness or UI-test-specific wrapper, keeping production rootContent free of
UITestConfig-driven debug routing.
Sources: Coding guidelines, Path instructions
This branch was successfully deployed
1 participant
Summary
Verification
Notes
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes iOS chat transcript snapping when expanding tool, terminal, or per‑command blocks by anchoring the first visible row and restoring it when content grows. Auto-scroll only triggers if the previous viewport was at the bottom, and viewport snapshots stay accurate after programmatic scrolls.
Bug Fixes
anchorBeforeLayout, extendedafterLayoutwith the old anchor, and manage a pending content-update anchor (cleared on user drag and ignored during data updates).Refactors
Written for commit 17122ec. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests