Repository navigation
Improve iOS GUI chat sending focus - #7068
Closed
azooz2003-bit wants to merge 6 commits into
Closed
azooz2003-bit wants to merge 6 commits into
azooz2003-bit wants to merge 6 commits into
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds outbound focus tracking through the chat store and transcript views, updates keyboard and table layout to position the sent message near the top, replaces the working indicator, adds focus and placement tests, and expands mobile chat and terminal send diagnostics. ChangesOutbound Focus and Transcript Layout
Chat Send Diagnostics Logging
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 2 warnings)
✅ Passed checks (21 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 |
Contributor
Greptile SummaryThis PR replaces the typing indicator dots+timer bubble with a compact animated bloom (
Confidence Score: 4/5The send-focus happy path works correctly, but a scroll-position jitter during keyboard animations after send remains unremedied from the previous review round, and The core store logic and reconciliation transfers are correct and well-tested. The known edge cases from prior reviews — anchor restoration overwriting an active focus during spacer-height-change reloads, and focus re-snapping on every keyboard/inset transition after send — are still present in ChatTranscriptTableView and make positioning less reliable during keyboard animation. ChatTranscriptTableView.swift — the update() reload path and handleLayoutChange interaction around spacer-height changes; ChatKeyboardTrackingViewController.swift — hardcoded chrome-height constants. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant U as User
participant CS as ChatConversationStore
participant CV as ChatScreen/ListV
participant TV as ChatTranscriptTableView Coordinator
participant UT as ChatTranscriptUITableView
U->>CS: send(text:)
CS->>CS: "latestOutboundFocusRowID = pending-X"
CS-->>CV: Observable change propagates
CV->>TV: update(outboundFocusRowID: pending-X)
TV->>TV: "shouldFocusOutbound = true, activeOutboundFocusRowID = pending-X"
TV->>TV: makeItems(spacerHeight: viewport x 1.05)
TV->>UT: reloadData() + layoutIfNeeded()
UT-->>TV: handleLayoutChange → focusOutbound(pending-X)
TV->>UT: setContentOffset(nearTop, animated: false)
Note over CS,TV: Agent responds — reconciliation
CS->>CS: "latestOutboundFocusRowID = msg-Y"
CS-->>CV: Observable change propagates
CV->>TV: update(outboundFocusRowID: msg-Y)
TV->>TV: "shouldFocusOutbound = true, activeOutboundFocusRowID = msg-Y"
TV->>UT: reloadData() + layoutIfNeeded()
TV->>UT: focusOutbound(msg-Y, animated: false)
Note over TV,UT: Keyboard animation — inset changes
UT-->>TV: handleLayoutChange (isViewportInsetsExternallyDriven)
TV->>UT: focusOutbound(activeOutboundFocusRowID, animated: false)
%%{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"}}}%%
sequenceDiagram
participant U as User
participant CS as ChatConversationStore
participant CV as ChatScreen/ListV
participant TV as ChatTranscriptTableView Coordinator
participant UT as ChatTranscriptUITableView
U->>CS: send(text:)
CS->>CS: "latestOutboundFocusRowID = pending-X"
CS-->>CV: Observable change propagates
CV->>TV: update(outboundFocusRowID: pending-X)
TV->>TV: "shouldFocusOutbound = true, activeOutboundFocusRowID = pending-X"
TV->>TV: makeItems(spacerHeight: viewport x 1.05)
TV->>UT: reloadData() + layoutIfNeeded()
UT-->>TV: handleLayoutChange → focusOutbound(pending-X)
TV->>UT: setContentOffset(nearTop, animated: false)
Note over CS,TV: Agent responds — reconciliation
CS->>CS: "latestOutboundFocusRowID = msg-Y"
CS-->>CV: Observable change propagates
CV->>TV: update(outboundFocusRowID: msg-Y)
TV->>TV: "shouldFocusOutbound = true, activeOutboundFocusRowID = msg-Y"
TV->>UT: reloadData() + layoutIfNeeded()
TV->>UT: focusOutbound(msg-Y, animated: false)
Note over TV,UT: Keyboard animation — inset changes
UT-->>TV: handleLayoutChange (isViewportInsetsExternallyDriven)
TV->>UT: focusOutbound(activeOutboundFocusRowID, animated: false)
Reviews (5): Last reviewed commit: "Give iOS chat thinking indicator more ch..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+122
to
+131
| let shouldScrollToBottom = scrollToBottomRequest != lastScrollToBottomRequest | ||
| let shouldFocusOutbound = outboundFocusRowID != nil | ||
| && outboundFocusRowID != lastOutboundFocusRowID | ||
| lastScrollToBottomRequest = scrollToBottomRequest | ||
| if shouldScrollToBottom { | ||
| activeOutboundFocusRowID = nil | ||
| } | ||
| if shouldFocusOutbound { | ||
| lastOutboundFocusRowID = outboundFocusRowID | ||
| activeOutboundFocusRowID = outboundFocusRowID |
Contributor
There was a problem hiding this comment.
Coordinator.lastOutboundFocusRowID resets to nil every time the UIViewRepresentable is recreated (e.g., a NavigationStack pop/push back to the same chat). ChatConversationStore.latestOutboundFocusRowID is never cleared, so the first update call into the fresh coordinator evaluates shouldFocusOutbound = true and scrolls to the previously sent message — overriding normal scroll restoration and disorienting the user who expected to land where they left off.
A minimal fix is to treat a coordinator's initial call specially: on the very first update, set lastOutboundFocusRowID = outboundFocusRowID without acting on it (consuming the stale signal), or add a dedicated reset path in ChatConversationStore that clears latestOutboundFocusRowID when the surface attaches.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/ChatTypingIndicatorView.swift`:
- Around line 64-78: The ChatTypingIndicatorView currently still flips rotating
to true on appear even when accessibilityReduceMotion is enabled, causing the
arc to snap into its rotated state. Update the onAppear logic in
ChatTypingIndicatorView so rotating only becomes true when reduceMotion is
false, or otherwise keep the initial pose unchanged while motion reduction is
active. Keep the existing animation setup and use the rotating state plus
reduceMotion check to preserve the still appearance without the first-render
jump.
In
`@Packages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Store/ChatConversationStore.swift`:
- Around line 647-650: The focus-row handling in ChatConversationStore is only
remapping the pending-to-echo case, so authoritative removals can leave
latestOutboundFocusRowID pointing at a deleted row. Centralize the focus
transition/clear logic around the relevant ChatConversationStore paths,
especially discard(pendingID:), reset, and reconcileTerminalPending(against:),
and ensure they clear or remap the focus when the target is removed. Add
coverage in tests for each authoritative removal path so the transcript fails
closed when the focused row disappears.
🪄 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: f5e854bb-a931-44a0-b0c1-4cddfd950907
📒 Files selected for processing (9)
Packages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Store/ChatConversationStore.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Store/ChatTranscriptRow.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/ChatConversationStoreTests.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Screen/ChatScreen.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptListView.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptTableView.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTypingIndicatorView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/AgentChatDemoScreen.swiftios/cmuxUITests/cmuxUITests.swift
Comment on lines
+647
to
+650
| let removedRowID = ChatTranscriptRow.pendingOutboundRowID(for: removed.id) | ||
| if latestOutboundFocusRowID == removedRowID { | ||
| latestOutboundFocusRowID = ChatTranscriptRow.messageRowID(for: message.id) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clear the focus row on every authoritative removal path.
This only remaps the focus id for the pending→echo case. discard(pendingID:), .reset, and reconcileTerminalPending(against:) can still remove the focused row without clearing/remapping latestOutboundFocusRowID, which leaves the iOS transcript in a stale focused state with no valid target row. Please centralize the focus-row transition/clear logic and cover those paths in tests. As per path instructions, derived focus signals must be updated from authoritative state transitions and fail closed when the reliable signal disappears.
🤖 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/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Store/ChatConversationStore.swift`
around lines 647 - 650, The focus-row handling in ChatConversationStore is only
remapping the pending-to-echo case, so authoritative removals can leave
latestOutboundFocusRowID pointing at a deleted row. Centralize the focus
transition/clear logic around the relevant ChatConversationStore paths,
especially discard(pendingID:), reset, and reconcileTerminalPending(against:),
and ensure they clear or remap the focus when the target is removed. Add
coverage in tests for each authoritative removal path so the transcript fails
closed when the focused row disappears.
Source: Path instructions
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 304-314: The visible-top calculation in ChatTranscriptTableView is
mixing coordinate spaces by treating
ChatTranscriptUITableView.topChromeOverlayInset like a window-space Y value.
Update the logic around the targetY computation to keep the chrome inset in
table-frame coordinates, using the current table frame (tableFrameInWindow) plus
the larger of adjustedContentInset.top and topChromeOverlayInset, so the
outbound row cannot be focused under the floating chrome.
🪄 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: b5ba3e7f-7089-4103-849f-75519a6a1544
📒 Files selected for processing (4)
Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Screen/ChatKeyboardTrackingViewController.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptTableView.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptUITableView.swiftios/cmuxUITests/cmuxUITests.swift
Comment on lines
+304
to
+314
| let tableFrameInWindow = tableView.window.map { tableView.convert(tableView.bounds, to: $0) } | ||
| ?? tableView.frame | ||
| let presentationMinY = (tableView as? ChatTranscriptUITableView)? | ||
| .presentationFrameInWindow()? | ||
| .minY | ||
| ?? tableFrameInWindow.minY | ||
| let insetTopY = tableFrameInWindow.minY + tableView.adjustedContentInset.top | ||
| let chromeTopY = (tableView as? ChatTranscriptUITableView)?.topChromeOverlayInset ?? 0 | ||
| let visibleTopY = max(insetTopY, chromeTopY) | ||
| let targetY = clampedOffsetY( | ||
| rect.minY + presentationMinY - visibleTopY - visibleTopPadding, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the chrome inset in the same coordinate space as the table frame.
Line 312 compares topChromeOverlayInset as if it were a window-space Y, but this value is plumbed through applyTranscriptViewportInsets(...) as a content inset. When the transcript table itself is shifted in window coordinates, visibleTopY becomes too small and the outbound row can still focus under the floating chrome. Compute the visible top from the table’s current frame plus the larger inset instead.
Proposed fix
let visibleTopPadding: CGFloat = 18
let tableFrameInWindow = tableView.window.map { tableView.convert(tableView.bounds, to: $0) }
?? tableView.frame
let presentationMinY = (tableView as? ChatTranscriptUITableView)?
.presentationFrameInWindow()?
.minY
?? tableFrameInWindow.minY
- let insetTopY = tableFrameInWindow.minY + tableView.adjustedContentInset.top
- let chromeTopY = (tableView as? ChatTranscriptUITableView)?.topChromeOverlayInset ?? 0
- let visibleTopY = max(insetTopY, chromeTopY)
+ let chromeTopInset = (tableView as? ChatTranscriptUITableView)?.topChromeOverlayInset ?? 0
+ let visibleTopY = presentationMinY + max(tableView.adjustedContentInset.top, chromeTopInset)
let targetY = clampedOffsetY(
rect.minY + presentationMinY - visibleTopY - visibleTopPadding,
in: tableView
)📝 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 tableFrameInWindow = tableView.window.map { tableView.convert(tableView.bounds, to: $0) } | |
| ?? tableView.frame | |
| let presentationMinY = (tableView as? ChatTranscriptUITableView)? | |
| .presentationFrameInWindow()? | |
| .minY | |
| ?? tableFrameInWindow.minY | |
| let insetTopY = tableFrameInWindow.minY + tableView.adjustedContentInset.top | |
| let chromeTopY = (tableView as? ChatTranscriptUITableView)?.topChromeOverlayInset ?? 0 | |
| let visibleTopY = max(insetTopY, chromeTopY) | |
| let targetY = clampedOffsetY( | |
| rect.minY + presentationMinY - visibleTopY - visibleTopPadding, | |
| let visibleTopPadding: CGFloat = 18 | |
| let tableFrameInWindow = tableView.window.map { tableView.convert(tableView.bounds, to: $0) } | |
| ?? tableView.frame | |
| let presentationMinY = (tableView as? ChatTranscriptUITableView)? | |
| .presentationFrameInWindow()? | |
| .minY | |
| ?? tableFrameInWindow.minY | |
| let chromeTopInset = (tableView as? ChatTranscriptUITableView)?.topChromeOverlayInset ?? 0 | |
| let visibleTopY = presentationMinY + max(tableView.adjustedContentInset.top, chromeTopInset) | |
| let targetY = clampedOffsetY( | |
| rect.minY + presentationMinY - visibleTopY - visibleTopPadding, |
🤖 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 304 - 314, The visible-top calculation in ChatTranscriptTableView
is mixing coordinate spaces by treating
ChatTranscriptUITableView.topChromeOverlayInset like a window-space Y value.
Update the logic around the targetY computation to keep the chrome inset in
table-frame coordinates, using the current table frame (tableFrameInWindow) plus
the larger of adjustedContentInset.top and topChromeOverlayInset, so the
outbound row cannot be focused under the floating chrome.
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/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Store/ChatConversationStore.swift (1)
647-650: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDon't promote heuristic reconcile matches into authoritative focus targets.
latestOutboundFocusRowIDis now correctness-critical UI state, but these branches still inheritreconcilePending(against:)/reconcileTerminalPending(against:)'s best-effort text matching. A false-positive reconcile now focuses the wrong committed row instead of failing closed. Please gate the handoff on a structured pending→echo mapping from the send path, or clear focus when only a heuristic match is available. As per path instructions, "treat the focus target as correctness-critical UI state" with "an authoritative, structured source of truth" and "Prefer fail-closed behavior (don’t attempt focus) when the reliable mapping isn’t available."Also applies to: 723-727
🤖 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/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Store/ChatConversationStore.swift` around lines 647 - 650, The focus handoff in ChatConversationStore is still promoting heuristic reconcile matches into latestOutboundFocusRowID, which can point UI focus at the wrong committed message. Update the reconcile paths around the pending→echo logic (including the removedRowID checks in the affected branches and the later matching branch) so they only assign latestOutboundFocusRowID when there is an authoritative structured mapping from the send path, not when reconcilePending(against:) or reconcileTerminalPending(against:) only produced a best-effort text match. If that reliable mapping is unavailable, clear or leave focus unset rather than guessing.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/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Store/ChatConversationStore.swift`:
- Around line 647-650: The focus handoff in ChatConversationStore is still
promoting heuristic reconcile matches into latestOutboundFocusRowID, which can
point UI focus at the wrong committed message. Update the reconcile paths around
the pending→echo logic (including the removedRowID checks in the affected
branches and the later matching branch) so they only assign
latestOutboundFocusRowID when there is an authoritative structured mapping from
the send path, not when reconcilePending(against:) or
reconcileTerminalPending(against:) only produced a best-effort text match. If
that reliable mapping is unavailable, clear or leave focus unset rather than
guessing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c3cc3c7a-213e-4ce8-9eee-3ecf838b17ab
📒 Files selected for processing (5)
Packages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Store/ChatConversationStore.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Store/ChatTranscriptRow.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/ChatConversationStoreTests.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileChatEventSource.swiftSources/TerminalController+MobileChat.swift
Collaborator
Author
|
Closing: the v1 mobile chat surface this polishes is being removed wholesale in #7660 ahead of a from-scratch GUI rebuild. The uncommitted local edits from this branch's worktree are preserved at /tmp/fable-feat-scrap-agent-gui/sending-ux-uncommitted.patch. |
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
Dogfood
Tag: igui
Mac deeplink: http://127.0.0.1:17320/igui
iOS simulator tag: igui
Physical iPhone reload: blocked by missing ASC credentials and local signing team
Do not merge until the iOS GUI dogfood checks pass.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Improves iOS chat sending UX with a compact animated bloom indicator and reliable send focus that keeps the just-sent message near the top with room below, even under floating top chrome and the keyboard.
New Features
Bug Fixes
Written for commit 94248eb. Summary will update on new commits.
Summary by CodeRabbit