Preserve iOS chat scroll momentum - #7109
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds ChangesMomentum-aware scroll guard
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (23 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 |
![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 `@ios/cmuxUITests/cmuxUITests.swift`:
- Around line 1228-1245: The transcript swipe test is asserting only an early
`offsetY` increase, which can pass before the coordinator later snaps back
during deceleration. Update the
`waitForTranscriptMetrics`/`captureKeyboardEvidenceFrame` flow in `cmuxUITests`
to key off the authoritative structured scroll-state fields (`scrollDragging`
and `scrollDecelerating`) instead of a single offset jump, and make the
`XCTAssertGreaterThan` checks verify the transcript remains away from the live
tail while momentum is active or after it fully settles.
🪄 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: 4069a8a7-8b19-49bb-a03d-240c95a14cc3
📒 Files selected for processing (3)
Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptTableView.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptUITableView.swiftios/cmuxUITests/cmuxUITests.swift
Comment on lines
+1228
to
+1245
| let afterSwipe = try waitForTranscriptMetrics(table, timeout: 1.5) { | ||
| $0.offsetY > before.offsetY + 40 | ||
| } | ||
| captureKeyboardEvidenceFrame( | ||
| prefix: "scroll-deceleration-after", | ||
| index: 0, | ||
| startedAt: Date(), | ||
| metrics: afterSwipe | ||
| ) | ||
| XCTAssertGreaterThan( | ||
| afterSwipe.offsetY, | ||
| before.offsetY + 40, | ||
| "A fast transcript swipe should move through the chat history instead of being swallowed by parent gesture handling. before=\(before) after=\(afterSwipe)" | ||
| ) | ||
| XCTAssertGreaterThan( | ||
| afterSwipe.distanceFromBottom, | ||
| 80, | ||
| "A single fast swipe from the middle fixture must not snap to the live bottom. before=\(before) after=\(afterSwipe)" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Wait for momentum-state evidence, not just an early offset jump.
This goes green as soon as one post-swipe sample has a larger offsetY. That can still happen even if the coordinator later restores/snaps during deceleration, so the test does not actually pin the regression this PR is fixing. Please key the assertion off the new structured scroll-state fields (scrollDragging/scrollDecelerating) and verify the transcript stays away from the live tail while momentum is active, or after momentum fully settles.
Suggested direction
- let afterSwipe = try waitForTranscriptMetrics(table, timeout: 1.5) {
- $0.offsetY > before.offsetY + 40
- }
+ let duringMomentum = try waitForTranscriptMetrics(table, timeout: 1.5) {
+ ($0.scrollDragging || $0.scrollDecelerating)
+ && $0.offsetY > before.offsetY + 40
+ && $0.distanceFromBottom > 80
+ }
+ let settled = try waitForTranscriptMetrics(table, timeout: 2.5) {
+ !$0.scrollDragging && !$0.scrollDecelerating
+ }
...
- metrics: afterSwipe
+ metrics: duringMomentum
...
- afterSwipe.offsetY,
+ duringMomentum.offsetY,
before.offsetY + 40,
...
- "A fast transcript swipe should move through the chat history instead of being swallowed by parent gesture handling. before=\(before) after=\(afterSwipe)"
+ "A fast transcript swipe should keep control while user momentum is active. before=\(before) duringMomentum=\(duringMomentum) settled=\(settled)"
)
XCTAssertGreaterThan(
- afterSwipe.distanceFromBottom,
+ settled.distanceFromBottom,
80,
- "A single fast swipe from the middle fixture must not snap to the live bottom. before=\(before) after=\(afterSwipe)"
+ "A single fast swipe from the middle fixture must not snap back to the live bottom after momentum settles. before=\(before) duringMomentum=\(duringMomentum) settled=\(settled)"
)As per path instructions, "base logic on a single authoritative, structured source ... not on ... 'best effort' fallbacks." Based on learnings and the PR context, these new scroll-state fields are the authoritative momentum signal this regression test should assert against.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let afterSwipe = try waitForTranscriptMetrics(table, timeout: 1.5) { | |
| $0.offsetY > before.offsetY + 40 | |
| } | |
| captureKeyboardEvidenceFrame( | |
| prefix: "scroll-deceleration-after", | |
| index: 0, | |
| startedAt: Date(), | |
| metrics: afterSwipe | |
| ) | |
| XCTAssertGreaterThan( | |
| afterSwipe.offsetY, | |
| before.offsetY + 40, | |
| "A fast transcript swipe should move through the chat history instead of being swallowed by parent gesture handling. before=\(before) after=\(afterSwipe)" | |
| ) | |
| XCTAssertGreaterThan( | |
| afterSwipe.distanceFromBottom, | |
| 80, | |
| "A single fast swipe from the middle fixture must not snap to the live bottom. before=\(before) after=\(afterSwipe)" | |
| let duringMomentum = try waitForTranscriptMetrics(table, timeout: 1.5) { | |
| ($0.scrollDragging || $0.scrollDecelerating) | |
| && $0.offsetY > before.offsetY + 40 | |
| && $0.distanceFromBottom > 80 | |
| } | |
| let settled = try waitForTranscriptMetrics(table, timeout: 2.5) { | |
| !$0.scrollDragging && !$0.scrollDecelerating | |
| } | |
| captureKeyboardEvidenceFrame( | |
| prefix: "scroll-deceleration-after", | |
| index: 0, | |
| startedAt: Date(), | |
| metrics: duringMomentum | |
| ) | |
| XCTAssertGreaterThan( | |
| duringMomentum.offsetY, | |
| before.offsetY + 40, | |
| "A fast transcript swipe should keep control while user momentum is active. before=\(before) duringMomentum=\(duringMomentum) settled=\(settled)" | |
| ) | |
| XCTAssertGreaterThan( | |
| settled.distanceFromBottom, | |
| 80, | |
| "A single fast swipe from the middle fixture must not snap back to the live bottom after momentum settles. before=\(before) duringMomentum=\(duringMomentum) settled=\(settled)" | |
| ) |
🤖 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 1228 - 1245, The transcript
swipe test is asserting only an early `offsetY` increase, which can pass before
the coordinator later snaps back during deceleration. Update the
`waitForTranscriptMetrics`/`captureKeyboardEvidenceFrame` flow in `cmuxUITests`
to key off the authoritative structured scroll-state fields (`scrollDragging`
and `scrollDecelerating`) instead of a single offset jump, and make the
`XCTAssertGreaterThan` checks verify the transcript remains away from the live
tail while momentum is active or after it fully settles.
Source: Path instructions
Contributor
Greptile SummaryThis PR introduces
Confidence Score: 5/5Safe to merge once iOS dogfood approval is obtained per the PR description's own hold condition. The changes are minimal and self-contained: No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[SwiftUI update / layout change fires] --> B{isUserScrollMomentumActive?}
B -- Yes --> C[update: skip wasAtBottom auto-scroll and anchor restore]
B -- Yes --> D[handleLayoutChange: clear pendingAnchor, update bottom state and return]
B -- Yes --> E[applyTranscriptViewportInsets: apply inset values, recordCurrentViewport, clear isViewportInsetsExternallyDriven, return]
B -- No --> F{shouldScrollToBottom or wasAtBottom?}
F -- Yes --> G[scrollToBottom]
F -- No --> H{anchor available?}
H -- Yes --> I[restore anchor, set pendingContentUpdateAnchor]
H -- No --> J[updateBottomState only]
C --> J
D --> J
E --> K[UIKit handles offset compensation during momentum]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[SwiftUI update / layout change fires] --> B{isUserScrollMomentumActive?}
B -- Yes --> C[update: skip wasAtBottom auto-scroll and anchor restore]
B -- Yes --> D[handleLayoutChange: clear pendingAnchor, update bottom state and return]
B -- Yes --> E[applyTranscriptViewportInsets: apply inset values, recordCurrentViewport, clear isViewportInsetsExternallyDriven, return]
B -- No --> F{shouldScrollToBottom or wasAtBottom?}
F -- Yes --> G[scrollToBottom]
F -- No --> H{anchor available?}
H -- Yes --> I[restore anchor, set pendingContentUpdateAnchor]
H -- No --> J[updateBottomState only]
C --> J
D --> J
E --> K[UIKit handles offset compensation during momentum]
Reviews (2): Last reviewed commit: "Document transcript momentum inset handl..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+226
to
+230
| if tableView.isUserScrollMomentumActive { | ||
| pendingContentUpdateAnchor = nil | ||
| updateBottomState(from: tableView) | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
pendingContentUpdateAnchor = nil is redundant but may still be load-bearing for the deceleration-only window
scrollViewWillBeginDragging already clears pendingContentUpdateAnchor at drag start, and update() unconditionally sets it to nil on line 139 before checking momentum. In the common case this nil-out in handleLayoutChange is therefore a no-op. However, there is a narrow window: a data update that arrives mid-deceleration calls update(), clears the anchor (line 139), does not set it back (momentum active), and then a bounds-change layout fires before momentum settles — at that point this guard is the only thing preventing a stale anchor from leaking in. Worth a short comment to make the intent explicit so a future refactor doesn't drop it thinking it's purely defensive.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
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/ChatTranscriptUITableView.swift (1)
129-132: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRe-run inset restoration when scroll momentum ends.
applyTranscriptViewportInsetsskips pin/restore/clamp whileisUserScrollMomentumActiveis true, and this file has noscrollViewDidEndDragging/scrollViewDidEndDeceleratingpath to apply the deferred inset change afterward. Queue that reconciliation and apply it once momentum stops, or the transcript can remain offset after the last mid-gesture inset update.🤖 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/ChatTranscriptUITableView.swift` around lines 129 - 132, `ChatTranscriptUITableView.isUserScrollMomentumActive` currently blocks `applyTranscriptViewportInsets` during user momentum, but there is no follow-up path to reconcile the deferred inset change after scrolling ends. Add a deferred inset update flow in `ChatTranscriptUITableView` that records the pending pin/restore/clamp work while `isTracking`, `isDragging`, or `isDecelerating` is true, then applies it from the scroll-end callbacks such as `scrollViewDidEndDragging` and `scrollViewDidEndDecelerating` so the transcript is re-aligned once momentum stops.
🤖 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/ChatTranscriptUITableView.swift`:
- Around line 129-132: `ChatTranscriptUITableView.isUserScrollMomentumActive`
currently blocks `applyTranscriptViewportInsets` during user momentum, but there
is no follow-up path to reconcile the deferred inset change after scrolling
ends. Add a deferred inset update flow in `ChatTranscriptUITableView` that
records the pending pin/restore/clamp work while `isTracking`, `isDragging`, or
`isDecelerating` is true, then applies it from the scroll-end callbacks such as
`scrollViewDidEndDragging` and `scrollViewDidEndDecelerating` so the transcript
is re-aligned once momentum stops.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 60be1045-40e6-46b8-a5ea-8b824216e46a
📒 Files selected for processing (2)
Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptTableView.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptUITableView.swift
austinywang
added a commit
that referenced
this pull request
Jul 2, 2026
…values-for Resolved conflicts: - .github/swift-file-length-budget.tsv: regenerated via swift_file_length_budget.py --write-budget - ChatScrollEdgeCoordinator.swift, ChatTranscriptTableView.swift: took origin/main (main superseded this branch's inline #if compiler(>=6.2) glass-API guards with the applyScrollEdgeEffects helper + scroll-momentum work in #7072/#7109; branch predated it) - GhosttySurfaceView.swift: took origin/main and removed the now-orphaned GhosttySurfaceHandle.swift; main's GhosttySurfaceWorkQueue redesign (#7098) supersedes this branch's GhosttySurfaceHandle Sendable-wrapper approach for the same surface-pointer safety concern. Codex transcript payload (resolver realpath fix, service shutdown()/race guard, MobileShellComposite isolated deinit) preserved. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jul 3, 2026
This branch was successfully deployed
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
Verification
git diff --checkxcodebuild -workspace ios/cmux.xcworkspace -scheme cmux-ios -destination 'platform=iOS Simulator,id=C9D82663-886D-4EBD-92A0-96CF38A42FF3' -derivedDataPath /tmp/cmux-dcel-uitest -only-testing:cmuxUITests/cmuxUITests/testAgentChatTranscriptFastSwipeEvidence test\n- Screenshot evidence exported locally to/tmp/cmux-dcel-scroll-evidence/: before offset 767, after offset 1582, after distance from bottom 359px.\n\n## Dogfood\n- mac tag: dcel\n- iOS tag: dcel\n- simulator reload succeeded and auto-paired\n- iPhone reload unavailable because connected devices were unavailable todevicectl\n\nDo not merge until the iOS dogfood scroll behavior is approved.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Improves iOS chat scroll by preserving swipe momentum so fast swipes glide without snapping. Stops auto-follow and inset changes while the user is scrolling.
isUserScrollMomentumActive(tracking/dragging/decelerating) and gated bottom auto-follow, anchor restoration, and transcript inset application; preserves UIKit’s live inset compensation during momentum.testAgentChatTranscriptFastSwipeEvidence; debug metrics now includescrollTracking/scrollDragging/scrollDecelerating; documented momentum-based inset handling.Written for commit 3a4b50a. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests