Fix iOS chat scroll-to-bottom completion - #7287
azooz2003-bit wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughUpdates iOS chat transcript bottom-follow handling, adds transcript item/rendering configuration, introduces repeatable preview fixture seeding, and expands UI coverage for scroll-to-bottom behavior. ChangesExplicit bottom-follow scrolling and tests
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (24 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 |
Greptile SummaryThis PR fixes the iOS chat scroll-to-bottom bug where tapping the button during streaming layout churn or active scroll momentum would not reliably reach the true bottom. The fix replaces the previous optimistic
Confidence Score: 4/5Safe to merge for the scroll bug fix itself; one new debug-only file and three new debug methods land in production Sources paths guarded only by #if DEBUG, violating the team's policy against test seams in production source. The scroll-state machine change is well-tested and the core logic holds up — staging, animated glide, layout-change re-pinning, and cancellation all operate on measured geometry rather than timing guesses. The concern is that StreamingPreviewSeedConversation.swift is a new #if DEBUG-only file committed to production Sources/, and ChatTranscriptUITableView gains three new #if DEBUG-guarded methods whose sole consumers are UI tests, both violating the established cmux policy that such facilities belong in a test-support module or dedicated debug package rather than main Sources/. Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/StreamingPreviewSeedConversation.swift (new debug-only file in production Sources) and Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptUITableView.swift (new #if DEBUG record* methods extending the test-instrumentation channel). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
TAP["User taps scroll-to-bottom button\n(scrollToBottomRequest += 1)"]
SET_FLAGS["Set isExplicitBottomFollowActive = true\nisUserVisibleBottomFollowActive = true\nneedsInitialUserVisibleBottomStaging = true"]
RELOAD{shouldReload?}
STAGE{distance > visibleDistance?}
JUMP["Instant jump to targetY − visibleDistance\n(invisible staging)"]
ANIMATE["setContentOffset(targetY, animated: true)\n→ isUserVisibleBottomAnimationRunning = true"]
ANIM_END["scrollViewDidEndScrollingAnimation"]
AT_BOTTOM{distanceFromBottom ≤ threshold?}
REPIN["handleLayoutChange / data reload\n→ scrollToBottomForFollow (non-animated)"]
DRAG["scrollViewWillBeginDragging\n→ clear all follow flags"]
UPWARD["cancelBottomFollowForStableContentUpwardScroll\n(stable content + offset moved up)\n→ clear all follow flags"]
DONE["updateBottomState → isAtBottom = true\nisUserVisibleBottomFollowActive = false"]
TAP --> SET_FLAGS --> RELOAD
RELOAD -- "yes (layout churn)" --> REPIN --> STAGE
RELOAD -- "no" --> STAGE
STAGE -- "yes" --> JUMP --> ANIMATE
STAGE -- "no" --> ANIMATE
ANIMATE --> ANIM_END --> AT_BOTTOM
AT_BOTTOM -- "no (content grew)" --> REPIN
AT_BOTTOM -- "yes" --> DONE
REPIN -- "layout change while follow active" --> REPIN
DRAG --> DONE
UPWARD --> DONE
%%{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
TAP["User taps scroll-to-bottom button\n(scrollToBottomRequest += 1)"]
SET_FLAGS["Set isExplicitBottomFollowActive = true\nisUserVisibleBottomFollowActive = true\nneedsInitialUserVisibleBottomStaging = true"]
RELOAD{shouldReload?}
STAGE{distance > visibleDistance?}
JUMP["Instant jump to targetY − visibleDistance\n(invisible staging)"]
ANIMATE["setContentOffset(targetY, animated: true)\n→ isUserVisibleBottomAnimationRunning = true"]
ANIM_END["scrollViewDidEndScrollingAnimation"]
AT_BOTTOM{distanceFromBottom ≤ threshold?}
REPIN["handleLayoutChange / data reload\n→ scrollToBottomForFollow (non-animated)"]
DRAG["scrollViewWillBeginDragging\n→ clear all follow flags"]
UPWARD["cancelBottomFollowForStableContentUpwardScroll\n(stable content + offset moved up)\n→ clear all follow flags"]
DONE["updateBottomState → isAtBottom = true\nisUserVisibleBottomFollowActive = false"]
TAP --> SET_FLAGS --> RELOAD
RELOAD -- "yes (layout churn)" --> REPIN --> STAGE
RELOAD -- "no" --> STAGE
STAGE -- "yes" --> JUMP --> ANIMATE
STAGE -- "no" --> ANIMATE
ANIMATE --> ANIM_END --> AT_BOTTOM
AT_BOTTOM -- "no (content grew)" --> REPIN
AT_BOTTOM -- "yes" --> DONE
REPIN -- "layout change while follow active" --> REPIN
DRAG --> DONE
UPWARD --> DONE
Reviews (4): Last reviewed commit: "Fix iOS chat scroll-to-bottom glide" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private func streamingPreviewRepeatedMessages(_ messages: [ChatMessage], repeatCount: Int) -> [ChatMessage] { | ||
| var repeatedMessages: [ChatMessage] = [] | ||
| repeatedMessages.reserveCapacity(messages.count * repeatCount) | ||
| for repeatIndex in 0..<repeatCount { | ||
| for message in messages { | ||
| repeatedMessages.append( | ||
| ChatMessage( | ||
| id: "\(message.id)-repeat-\(repeatIndex)", | ||
| seq: repeatedMessages.count, | ||
| role: message.role, | ||
| timestamp: message.timestamp.addingTimeInterval(TimeInterval(repeatIndex * messages.count)), | ||
| kind: message.kind | ||
| ) | ||
| ) | ||
| } | ||
| } | ||
| return repeatedMessages | ||
| } |
There was a problem hiding this comment.
streamingPreviewRepeatedMessages is functionally identical to AgentChatDemoScreen.repeatedUITestMessagesIfNeeded (same ID/seq/timestamp construction logic). Both live in separate #if DEBUG production-source files and must be kept in sync by hand. A change to ID-namespacing or timestamp offsets would need to land in two places, and the two implementations are already subtly different in their outer contract (AgentChatDemoScreen reads the env-var and guards inside the method; StreamingChatPreviewView exposes a separate streamingPreviewUITestFixtureRepeatCount() accessor for the same env-var). Factoring the shared algorithm into a single file-private helper used by both callers would remove the drift surface.
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!
| private func scrollToBottom(in tableView: UITableView, animated: Bool) { | ||
| tableView.layoutIfNeeded() | ||
| cancelUserScrollMomentumIfNeeded(in: tableView) | ||
| let targetY = maxOffsetY(in: tableView) | ||
| tableView.setContentOffset(CGPoint(x: tableView.contentOffset.x, y: targetY), animated: animated) | ||
| (tableView as? ChatTranscriptUITableView)?.recordCurrentViewport() | ||
| setAtBottom(true) | ||
| updateBottomState(from: tableView) | ||
| } | ||
|
|
||
| private func cancelUserScrollMomentumIfNeeded(in tableView: UITableView) { | ||
| guard tableView.isTracking || tableView.isDragging || tableView.isDecelerating else { return } | ||
| tableView.setContentOffset(tableView.contentOffset, animated: false) | ||
| } |
There was a problem hiding this comment.
cancelUserScrollMomentumIfNeeded name vs guard scope mismatch
The name implies the function cancels in-flight deceleration, but the guard also triggers on isTracking and isDragging — states where the user's finger is still actively on the table. Setting contentOffset to cancel those touch states is valid UIKit (it terminates the internal pan recognizer), but callers reading this name expect it to be a no-op when the user has an active touch. The broader guard is intentional for the button-tap-during-active-drag scenario, so at minimum the name and/or the inline comment should reflect that it also cancels live-touch gestures, not just decelerating momentum.
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!
| private var isExplicitBottomFollowActive = false | ||
| private var lastObservedScrollContentHeight: CGFloat? | ||
| private var lastObservedScrollOffsetY: CGFloat? |
There was a problem hiding this comment.
isExplicitBottomFollowActive has no success-based terminal state
Once set, isExplicitBottomFollowActive is only cleared by scrollViewWillBeginDragging or cancelBottomFollowForStableContentUpwardScroll; there is no path that clears it when the table actually arrives and remains stably at the bottom. After streaming ends and the content is stable at the bottom, the flag stays true indefinitely. Every subsequent layout change — cell-size churn, rotation, keyboard show/hide — enters the isExplicitBottomFollowActive branch in handleLayoutChange and calls scrollToBottom instead of the normal restoreKeyboardViewport/wasAtBottom path. Functionally these produce the same offset when wasAtBottom == true, so there is no correctness bug today, but the state machine has no clean exit once success is reached. Clearing the flag inside updateBottomState when it observes distanceFromBottom == 0 would make the lifecycle explicit and remove the redundant scrollToBottom calls on stable-at-bottom layout events.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 2007-2024: The streaming chat preview test flag is set in
launchStreamingChatPreviewApp, but the app root never consumes
CMUX_UITEST_STREAMING_CHAT_PREVIEW, so the intended preview path is never
activated. Update the app entry point/root scene selection to read this launch
environment value and route into the StreamingChatPreviewView branch when it is
present, using the existing launchApp and app root wiring as the place to add
the check; if that branch is not meant to exist, remove the unused flag from the
test helper instead.
In
`@Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptTableView.swift`:
- Around line 86-88: `isExplicitBottomFollowActive` is never reset when the
transcript actually reaches the bottom, so the bottom-follow state can stay
stuck on and keep forcing `scrollToBottom`/momentum cancellation during later
idle reloads. Update the state flow in `ChatTranscriptTableView` so the flag is
explicitly cleared when `updateBottomState` or the `scrollToBottom` path
confirms the view has settled at the measured bottom, while still clearing it on
user-initiated scroll changes via `scrollViewWillBeginDragging` and
`cancelBottomFollowForStableContentUpwardScroll`. Make the transition match the
intended invariant: active until bottom is reached or the user scrolls back.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/AgentChatDemoScreen.swift`:
- Around line 191-217: The fixture-repeat logic in AgentChatDemoScreen is
duplicated with StreamingChatPreviewView and should be centralized. Extract the
shared repeat behavior from repeatedUITestMessagesIfNeeded and
streamingPreviewRepeatedMessages into one helper or ChatFixtureConversation
extension that handles the CMUX_UITEST_AGENT_CHAT_FIXTURE_REPEAT_COUNT env var,
id/seq generation, and timestamp offset. Update both preview screens to call
that shared helper so the repeat contract stays consistent.
🪄 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: e4d9a962-7966-47be-ad8f-45aba5f29423
📒 Files selected for processing (5)
Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptListView.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptTableView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/AgentChatDemoScreen.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/StreamingChatPreviewView.swiftios/cmuxUITests/cmuxUITests.swift
💤 Files with no reviewable changes (1)
- Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptListView.swift
| @MainActor | ||
| private func launchStreamingChatPreviewApp(environment: [String: String] = [:]) -> XCUIApplication { | ||
| var launchEnvironment = [ | ||
| "CMUX_UITEST_STREAMING_CHAT_PREVIEW": "1", | ||
| ] | ||
| for (key, value) in environment { | ||
| launchEnvironment[key] = value | ||
| } | ||
| let app = launchApp(mockData: false, environment: launchEnvironment) | ||
| let table = app.tables["ChatTranscriptTableView"] | ||
| XCTAssertTrue(table.waitForExistence(timeout: 8)) | ||
| XCTAssertTrue( | ||
| settleChatPreviewKeyboardDown(in: app, table: table), | ||
| "Streaming chat preview must start keyboard-down before scroll evidence is collected. metrics=\(String(describing: transcriptMetrics(from: table)))" | ||
| ) | ||
| return app | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'CMUX_UITEST_STREAMING_CHAT_PREVIEW' --type=swiftRepository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n-- streaming preview symbol search --\n'
rg -n 'StreamingChatPreviewView|CMUX_UITEST_STREAMING_CHAT_PREVIEW|ChatTranscriptTableView' ios -g '*.swift'
printf '\n-- probable app entry files --\n'
fd -a 'App.swift|main.swift|Scene.swift|ContentView.swift' ios
printf '\n-- inspect app entry candidates (first 250 lines each) --\n'
for f in $(fd -a 'App.swift|main.swift|Scene.swift|ContentView.swift' ios); do
echo "### $f"
sed -n '1,250p' "$f"
doneRepository: manaflow-ai/cmux
Length of output: 17210
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n-- search for any streaming-preview or env plumbing outside the earlier hits --\n'
rg -n 'StreamingChatPreviewView|CMUX_UITEST_STREAMING_CHAT_PREVIEW|ChatTranscriptTableView|UITestConfig|launchApp\(' ios cmux -g '*.swift'
printf '\n-- inspect the rest of the mobile root scene branch --\n'
sed -n '250,420p' /home/jailuser/git/ios/cmuxPackage/Sources/cmuxFeature/CMUXMobileRootScene.swift
printf '\n-- inspect launchApp helper in UI tests --\n'
sed -n '1,220p' ios/cmuxUITests/cmuxUITests.swiftRepository: manaflow-ai/cmux
Length of output: 3999
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n-- UITestConfig definitions and env keys --\n'
rg -n 'struct UITestConfig|enum UITestConfig|workspaceListLayoutPreviewEnabled|mockDataEnabled|dogfoodAttachURL|attachURL|CMUX_ZOOM_STRESS|CMUX_BOTTOM_SCROLL_STRESS|STREAMING_CHAT' ios cmuxPackage -g '*.swift'
printf '\n-- inspect UITestConfig and mobile root scene around the branching logic --\n'
for f in \
ios/cmux/AppCompositionRoot.swift \
ios/cmuxPackage/Sources/cmuxFeature/CMUXMobileRootScene.swift \
ios/cmuxUITests/cmuxUITests.swift
do
echo "### $f"
wc -l "$f"
done
printf '\n-- cmuxUITests launch helper --\n'
sed -n '2050,2125p' ios/cmuxUITests/cmuxUITests.swift
printf '\n-- CMUXMobileRootScene branching around the debug previews --\n'
sed -n '230,340p' ios/cmuxPackage/Sources/cmuxFeature/CMUXMobileRootScene.swiftRepository: manaflow-ai/cmux
Length of output: 7278
Wire CMUX_UITEST_STREAMING_CHAT_PREVIEW into the app root. The test launcher sets this flag, but the app entry point never reads it and there’s no StreamingChatPreviewView branch, so this path won’t enter the intended preview state. Add the matching root-scene branch or remove the unused flag.
🤖 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 2007 - 2024, The streaming
chat preview test flag is set in launchStreamingChatPreviewApp, but the app root
never consumes CMUX_UITEST_STREAMING_CHAT_PREVIEW, so the intended preview path
is never activated. Update the app entry point/root scene selection to read this
launch environment value and route into the StreamingChatPreviewView branch when
it is present, using the existing launchApp and app root wiring as the place to
add the check; if that branch is not meant to exist, remove the unused flag from
the test helper instead.
483dd4e to
8ab674a
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/AgentChatDemoScreen.swift (1)
191-217: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFixture-repeat logic still duplicated (now in
StreamingPreviewSeedConversation.swift).
repeatedUITestMessagesIfNeededis functionally identical touiTestFixtureRepeatCount()/repeatedMessages(_:repeatCount:)in the newStreamingPreviewSeedConversation.swift(same env var, same id/seq/timestamp derivation). A previous review already flagged this duplication when the logic lived inline inStreamingChatPreviewView.swift; it has since moved rather than been consolidated. Both files are in the same target — extract one shared helper (e.g. aChatFixtureConversationextension or a small file-scope utility struct) so the repeat contract can't drift between the demo screen and the streaming preview.♻️ Suggested consolidation direction
- private func repeatedUITestMessagesIfNeeded(_ messages: [ChatMessage]) -> [ChatMessage] { - let env = ProcessInfo.processInfo.environment - guard let rawRepeatCount = env["CMUX_UITEST_AGENT_CHAT_FIXTURE_REPEAT_COUNT"]? - .trimmingCharacters(in: .whitespacesAndNewlines), - let repeatCount = Int(rawRepeatCount), - repeatCount > 1 - else { - return messages - } - var repeatedMessages: [ChatMessage] = [] - repeatedMessages.reserveCapacity(messages.count * repeatCount) - for repeatIndex in 0..<repeatCount { - for message in messages { - repeatedMessages.append( - ChatMessage( - id: "\(message.id)-repeat-\(repeatIndex)", - seq: repeatedMessages.count, - role: message.role, - timestamp: message.timestamp.addingTimeInterval(TimeInterval(repeatIndex * messages.count)), - kind: message.kind - ) - ) - } - } - return repeatedMessages - } + private func repeatedUITestMessagesIfNeeded(_ messages: [ChatMessage]) -> [ChatMessage] { + UITestFixtureRepeater.repeatedIfNeeded( + messages, + environment: ProcessInfo.processInfo.environment + ) + }🤖 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/AgentChatDemoScreen.swift` around lines 191 - 217, The fixture-repeat logic in repeatedUITestMessagesIfNeeded duplicates the same CMUX_UITEST_AGENT_CHAT_FIXTURE_REPEAT_COUNT handling and repeated message construction already present in uiTestFixtureRepeatCount()/repeatedMessages(_:repeatCount:), so consolidate it into one shared helper used by AgentChatDemoScreen and StreamingPreviewSeedConversation. Extract the repeat-count parsing and ChatMessage cloning logic into a common utility or extension in the shared target, then have repeatedUITestMessagesIfNeeded call that helper instead of maintaining a separate implementation.
🤖 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.
Duplicate comments:
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/AgentChatDemoScreen.swift`:
- Around line 191-217: The fixture-repeat logic in
repeatedUITestMessagesIfNeeded duplicates the same
CMUX_UITEST_AGENT_CHAT_FIXTURE_REPEAT_COUNT handling and repeated message
construction already present in
uiTestFixtureRepeatCount()/repeatedMessages(_:repeatCount:), so consolidate it
into one shared helper used by AgentChatDemoScreen and
StreamingPreviewSeedConversation. Extract the repeat-count parsing and
ChatMessage cloning logic into a common utility or extension in the shared
target, then have repeatedUITestMessagesIfNeeded call that helper instead of
maintaining a separate implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1acd5f2c-e072-45ba-a0f3-e15193438ff2
📒 Files selected for processing (6)
Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptListView.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptTableView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/AgentChatDemoScreen.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/StreamingChatPreviewView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/StreamingPreviewSeedConversation.swiftios/cmuxUITests/cmuxUITests.swift
💤 Files with no reviewable changes (1)
- Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptListView.swift
8ab674a to
9f4cf66
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)
231-276: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the repeated
isExplicitBottomFollowActivecheck inhandleLayoutChange.The same
if isExplicitBottomFollowActive { scrollToBottom(in: tableView, animated: false) }appears three times (Lines 247-249, 255-258, 263-264) with subtly different follow-up behavior aroundpendingContentUpdateAnchorclearing and early return. The third occurrence doesn't clearpendingContentUpdateAnchor, unlike the other branches — please double check that a stale anchor from an earlier non-follow restore can't later be replayed by thecontentChanged, let pendingContentUpdateAnchorbranch (Line 267) once follow is cancelled.♻️ Sketch of consolidation
private func handleLayoutChange(...) { guard !isHandlingLayout else { return } ... isHandlingLayout = true defer { isHandlingLayout = false } + if isExplicitBottomFollowActive { + scrollToBottom(in: tableView, animated: false) + pendingContentUpdateAnchor = nil + updateBottomState(from: tableView) + return + } if tableView.isUserScrollMomentumActive { - if isExplicitBottomFollowActive { - scrollToBottom(in: tableView, animated: false) - } pendingContentUpdateAnchor = nil updateBottomState(from: tableView) return } if tableView.isViewportInsetsExternallyDriven || isApplyingDataUpdate { - if isExplicitBottomFollowActive { - scrollToBottom(in: tableView, animated: false) - return - } updateBottomState(from: tableView) return } - if isExplicitBottomFollowActive { - scrollToBottom(in: tableView, animated: false) - } else if boundsChanged, let oldViewport { + if boundsChanged, let oldViewport { restoreKeyboardViewport(snapshot: oldViewport, in: tableView) } else if contentChanged, let pendingContentUpdateAnchor { ...🤖 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 231 - 276, The handleLayoutChange logic in ChatTranscriptTableView repeats the isExplicitBottomFollowActive scrollToBottom path in multiple branches, and the branch after isViewportInsetsExternallyDriven or isApplyingDataUpdate does not clear pendingContentUpdateAnchor. Consolidate the bottom-follow handling into a single consistent path in handleLayoutChange, and make sure pendingContentUpdateAnchor is cleared whenever you scroll to bottom in explicit follow mode so a stale anchor cannot be restored later by the contentChanged/pendingContentUpdateAnchor branch.
🤖 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 231-276: The handleLayoutChange logic in ChatTranscriptTableView
repeats the isExplicitBottomFollowActive scrollToBottom path in multiple
branches, and the branch after isViewportInsetsExternallyDriven or
isApplyingDataUpdate does not clear pendingContentUpdateAnchor. Consolidate the
bottom-follow handling into a single consistent path in handleLayoutChange, and
make sure pendingContentUpdateAnchor is cleared whenever you scroll to bottom in
explicit follow mode so a stale anchor cannot be restored later by the
contentChanged/pendingContentUpdateAnchor branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5d5a0c30-92ab-4b5b-ab4b-5d2308de946f
📒 Files selected for processing (4)
Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptListView.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptTableConfiguration.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptTableItem.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptTableView.swift
💤 Files with no reviewable changes (1)
- Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptListView.swift
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)
237-254: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftTrack scroll origin explicitly instead of inferring user takeover from offset deltas.
scrollViewWillBeginDraggingalready clears follow for real drags, so this fallback is only handling non-drag scroll-origin changes. ThecontentSize-stable +contentOffset.ydelta check is still an approximation and can disable follow on other upward offset changes; thread a dedicated programmatic-scroll/origin flag through thesetContentOffsetpaths instead.🤖 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 237 - 254, The fallback in cancelBottomFollowForStableContentUpwardScroll(in:) is still inferring user takeover from contentOffset/contentSize deltas, which can clear bottom-follow for non-drag upward movements. Replace that heuristic with an explicit scroll-origin flag threaded through the setContentOffset-related paths in ChatTranscriptTableView, and use it here to only disable follow for genuine non-drag programmatic-origin transitions; keep scrollViewWillBeginDragging as the direct path for real user drags.Source: Path instructions
♻️ Duplicate comments (4)
ios/cmuxUITests/cmuxUITests.swift (1)
2118-2135: 🎯 Functional Correctness | 🟠 MajorVerify
CMUX_UITEST_STREAMING_CHAT_PREVIEWis actually wired into the app root.A prior review on this exact helper found the launch environment flag was set here but never consumed by the app entry point, with no
StreamingChatPreviewViewbranch to enter — meaningtestAgentChatScrollToBottomButtonReturnsDuringStreamingLayoutChurn(which depends on this launcher) may not actually exercise real streaming/layout churn. Unlike the other flagged item in this stack, this one has no "Addressed" annotation from a later commit.#!/bin/bash # Description: confirm the streaming-preview launch flag is consumed by the app root. rg -n 'CMUX_UITEST_STREAMING_CHAT_PREVIEW|StreamingChatPreviewView' --type=swift🤖 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 2118 - 2135, The launch flag set by launchStreamingChatPreviewApp is not enough unless the app root actually reads it and routes into StreamingChatPreviewView. Update the app entry/root flow to consume CMUX_UITEST_STREAMING_CHAT_PREVIEW and branch into the streaming chat preview path so testAgentChatScrollToBottomButtonReturnsDuringStreamingLayoutChurn exercises real streaming/layout churn. Use the existing symbols launchStreamingChatPreviewApp, CMUX_UITEST_STREAMING_CHAT_PREVIEW, and StreamingChatPreviewView to locate and wire the missing app-root entry point.Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptTableView.swift (1)
166-172: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
isExplicitBottomFollowActivestill never clears when the transcript actually reaches bottom.The reset happens only in
scrollViewWillBeginDragging(line 216) andcancelBottomFollowForStableContentUpwardScroll(249) — i.e. only on the "user scrolls back" half of the invariant.updateBottomState(430-438) clears the siblingisUserVisibleBottomFollowActive/needsInitialUserVisibleBottomStagingwhenisBottombut leavesisExplicitBottomFollowActiveuntouched, andscrollViewDidEndScrollingAnimation's "settled" else-branch (230-233) does the same omission. A prior review flagged exactly this and was marked addressed, but the reset is still absent here.Concretely, this means once a scroll-to-bottom tap fires,
isExplicitBottomFollowActivestaystrueindefinitely at rest, and every future reload while idling at the bottom takes theisExplicitBottomFollowActive || (wasAtBottom && …)branch (166) purely because of the stale flag rather than the intendedwasAtBottom-driven condition — an invalid-but-representable state the invariant is supposed to prevent.♻️ Suggested fix: clear the flag at the single canonical "reached bottom" point
private func updateBottomState(from tableView: UITableView) { let isBottom = distanceFromBottom(in: tableView) <= chatTranscriptAtBottomThreshold setAtBottom(isBottom) if isBottom, !isUserVisibleBottomAnimationRunning { + isExplicitBottomFollowActive = false isUserVisibleBottomFollowActive = false needsInitialUserVisibleBottomStaging = false } syncDebugBottomFollowState(in: tableView) }As per path instructions, "Ensure there is exactly one authoritative source of truth for whether follow mode is active... Avoid any correctness-critical fallback branches based on heuristics... prefer prompt state reads and fail-closed behavior."
Also applies to: 223-235, 430-438
🤖 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 166 - 172, `isExplicitBottomFollowActive` is not being cleared when the transcript actually reaches the bottom, leaving follow mode stuck active after a scroll-to-bottom completes. Update the canonical “reached bottom” handling in `ChatTranscriptTableView`—specifically `updateBottomState` and the settled path in `scrollViewDidEndScrollingAnimation`—to reset `isExplicitBottomFollowActive` alongside the existing bottom-follow flags when `isBottom` is true. Keep `scrollViewWillBeginDragging` and `cancelBottomFollowForStableContentUpwardScroll` as the user-cancel paths, but make the bottom-settled path the single authoritative place that clears this state.Source: Path instructions
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/AgentChatDemoScreen.swift (1)
191-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFixture limit/repeat logic still duplicated with
StreamingPreviewSeedConversation.swift, and duplication has grown.A prior review flagged
repeatedUITestMessagesIfNeededas duplicate of the repeat logic inStreamingPreviewSeedConversation.swiftand marked it addressed, but the duplication is still present here. This diff additionally introduceslimitedUITestMessagesIfNeeded, which duplicates the newlimitedMessageshelper inStreamingPreviewSeedConversation.swift— same env var, same guard conditions, same prefix logic. Both files live in the same target; extracting a shared helper (e.g., a small file-scope utility or extension onChatFixtureConversation) would prevent the limit/repeat contract from silently drifting between the two preview/demo screens as it has now doubled in surface area.🤖 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/AgentChatDemoScreen.swift` around lines 191 - 234, The fixture limit/repeat behavior is duplicated between AgentChatDemoScreen and StreamingPreviewSeedConversation, including both the repeat and new limit helpers. Move the shared environment-driven logic out of uiTestMessagesIfNeeded, limitedUITestMessagesIfNeeded, and repeatedUITestMessagesIfNeeded into one common helper or extension used by both screens, so the CMUX_UITEST_AGENT_CHAT_FIXTURE_MESSAGE_COUNT and CMUX_UITEST_AGENT_CHAT_FIXTURE_REPEAT_COUNT handling stays consistent in one place.Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/StreamingPreviewSeedConversation.swift (1)
30-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCounterpart of the duplicated fixture limit/repeat logic.
limitedMessages/uiTestFixtureRepeatCount/repeatedMessageshere mirrorlimitedUITestMessagesIfNeeded/repeatedUITestMessagesIfNeededinAgentChatDemoScreen.swiftalmost line-for-line. See the consolidated recommendation left on that file.🤖 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/StreamingPreviewSeedConversation.swift` around lines 30 - 65, The fixture limiting/repeating logic in limitedMessages, uiTestFixtureRepeatCount, and repeatedMessages is duplicated almost exactly with the helpers in AgentChatDemoScreen.swift. Consolidate this behavior into a shared utility or reusable helper so both StreamingPreviewSeedConversation and AgentChatDemoScreen call the same implementation, keeping the environment parsing and message repetition logic in one place.
🤖 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 237-254: The fallback in
cancelBottomFollowForStableContentUpwardScroll(in:) is still inferring user
takeover from contentOffset/contentSize deltas, which can clear bottom-follow
for non-drag upward movements. Replace that heuristic with an explicit
scroll-origin flag threaded through the setContentOffset-related paths in
ChatTranscriptTableView, and use it here to only disable follow for genuine
non-drag programmatic-origin transitions; keep scrollViewWillBeginDragging as
the direct path for real user drags.
---
Duplicate comments:
In `@ios/cmuxUITests/cmuxUITests.swift`:
- Around line 2118-2135: The launch flag set by launchStreamingChatPreviewApp is
not enough unless the app root actually reads it and routes into
StreamingChatPreviewView. Update the app entry/root flow to consume
CMUX_UITEST_STREAMING_CHAT_PREVIEW and branch into the streaming chat preview
path so testAgentChatScrollToBottomButtonReturnsDuringStreamingLayoutChurn
exercises real streaming/layout churn. Use the existing symbols
launchStreamingChatPreviewApp, CMUX_UITEST_STREAMING_CHAT_PREVIEW, and
StreamingChatPreviewView to locate and wire the missing app-root entry point.
In
`@Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptTableView.swift`:
- Around line 166-172: `isExplicitBottomFollowActive` is not being cleared when
the transcript actually reaches the bottom, leaving follow mode stuck active
after a scroll-to-bottom completes. Update the canonical “reached bottom”
handling in `ChatTranscriptTableView`—specifically `updateBottomState` and the
settled path in `scrollViewDidEndScrollingAnimation`—to reset
`isExplicitBottomFollowActive` alongside the existing bottom-follow flags when
`isBottom` is true. Keep `scrollViewWillBeginDragging` and
`cancelBottomFollowForStableContentUpwardScroll` as the user-cancel paths, but
make the bottom-settled path the single authoritative place that clears this
state.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/AgentChatDemoScreen.swift`:
- Around line 191-234: The fixture limit/repeat behavior is duplicated between
AgentChatDemoScreen and StreamingPreviewSeedConversation, including both the
repeat and new limit helpers. Move the shared environment-driven logic out of
uiTestMessagesIfNeeded, limitedUITestMessagesIfNeeded, and
repeatedUITestMessagesIfNeeded into one common helper or extension used by both
screens, so the CMUX_UITEST_AGENT_CHAT_FIXTURE_MESSAGE_COUNT and
CMUX_UITEST_AGENT_CHAT_FIXTURE_REPEAT_COUNT handling stays consistent in one
place.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/StreamingPreviewSeedConversation.swift`:
- Around line 30-65: The fixture limiting/repeating logic in limitedMessages,
uiTestFixtureRepeatCount, and repeatedMessages is duplicated almost exactly with
the helpers in AgentChatDemoScreen.swift. Consolidate this behavior into a
shared utility or reusable helper so both StreamingPreviewSeedConversation and
AgentChatDemoScreen call the same implementation, keeping the environment
parsing and message repetition logic in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0e17298e-b27d-4c9b-8186-e892861b5e4a
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (6)
Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptTableView+ScrollGeometry.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptTableView.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptUITableView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/AgentChatDemoScreen.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/StreamingPreviewSeedConversation.swiftios/cmuxUITests/cmuxUITests.swift
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
3 participants
Summary
Fix mechanism
The table coordinator now treats a scroll-to-bottom tap as an explicit bottom-follow state. Layout reloads, content-height growth, and cell size churn re-pin to the real bottom while that state is active. User dragging, or stable-content upward movement, cancels the follow. Normal no-reload SwiftUI updates at the bottom avoid repeated no-op scroll work.
This is a principled fix because scroll ownership is modeled from measured table geometry instead of timing or a one-shot offset write.
Repro
Before the fix,
testAgentChatScrollToBottomButtonReturnsDuringStreamingLayoutChurnfailed with the transcript still far from bottom after tapping the button:/tmp/cmux-ios-fixscroll-streaming-red.xcresult5290.33Verification
/tmp/cmux-ios-fixscroll-focused-final3.xcresult, 3 passed, 0 failed.483dd4e4b53314f1f5a591bd08e137c194d90291based on identical touched Swift/UI-test content and/tmp/cmux-ios-fixscroll-verifier-57a388.xcresult, 3 passed, 0 failed./tmp/cmux-assets/fix-ios-scroll-bottom/scroll-bottom-streaming-tap/contact-sheet.png.origin/main, no accepted/actionable findings, no cmux-policy findings.Residual risk: the passing UI-test result bundles still contain pre-existing SwiftUI runtime warnings about modifying state during view update. This PR does not introduce or fix that warning class.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes iOS chat scroll-to-bottom so a tap reliably reaches the true bottom with a visible final glide, even during streaming churn and fast-swipe momentum. The button stays visible until the transcript is actually at bottom.
Bug Fixes
isAtBottomwrites.StreamingPreviewSeedConversationand supportsCMUX_UITEST_AGENT_CHAT_FIXTURE_MESSAGE_COUNTandCMUX_UITEST_AGENT_CHAT_FIXTURE_REPEAT_COUNT.Refactors
ChatTranscriptTableConfiguration,ChatTranscriptTableItem, and scroll geometry helpers into dedicated files to simplify the table view.Written for commit 2f34f80. Summary will update on new commits.
Summary by CodeRabbit