Repository navigation
Improve iOS GUI chat sending focus #7068
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
bdacba8
5ffb836
765217e
f2a9edf
c49fd13
94248eb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ struct ChatTranscriptTableView: UIViewRepresentable { | |
| let onRetryInitialLoad: () -> Void | ||
| @Binding var isAtBottom: Bool | ||
| let scrollToBottomRequest: Int | ||
| let outboundFocusRowID: String? | ||
|
|
||
| @Environment(\.chatTheme) private var theme | ||
| @Environment(\.chatMarkdownRenderer) private var markdownRenderer | ||
|
|
@@ -72,7 +73,8 @@ struct ChatTranscriptTableView: UIViewRepresentable { | |
| contentCache: contentCache | ||
| ), | ||
| in: tableView, | ||
| scrollToBottomRequest: scrollToBottomRequest | ||
| scrollToBottomRequest: scrollToBottomRequest, | ||
| outboundFocusRowID: outboundFocusRowID | ||
| ) | ||
| } | ||
|
|
||
|
|
@@ -83,6 +85,8 @@ struct ChatTranscriptTableView: UIViewRepresentable { | |
| private var agentState: ChatAgentState = .idle | ||
| private var topRequestKey: String? | ||
| private var lastScrollToBottomRequest = 0 | ||
| private var lastOutboundFocusRowID: String? | ||
| private var activeOutboundFocusRowID: String? | ||
| private var isHandlingLayout = false | ||
| private weak var tableView: ChatTranscriptUITableView? | ||
| private var isAtBottom: Binding<Bool> | ||
|
|
@@ -111,22 +115,38 @@ struct ChatTranscriptTableView: UIViewRepresentable { | |
| fileprivate func update( | ||
| configuration: ChatTranscriptTableConfiguration, | ||
| in tableView: ChatTranscriptUITableView, | ||
| scrollToBottomRequest: Int | ||
| scrollToBottomRequest: Int, | ||
| outboundFocusRowID: String? | ||
| ) { | ||
| self.configuration = configuration | ||
| let nextItems = configuration.makeItems() | ||
| let shouldScrollToBottom = scrollToBottomRequest != lastScrollToBottomRequest | ||
| let shouldFocusOutbound = outboundFocusRowID != nil | ||
| && outboundFocusRowID != lastOutboundFocusRowID | ||
| lastScrollToBottomRequest = scrollToBottomRequest | ||
| if shouldScrollToBottom { | ||
| activeOutboundFocusRowID = nil | ||
| } | ||
| if shouldFocusOutbound { | ||
| lastOutboundFocusRowID = outboundFocusRowID | ||
| activeOutboundFocusRowID = outboundFocusRowID | ||
|
Comment on lines
+122
to
+131
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
A minimal fix is to treat a coordinator's initial call specially: on the very first |
||
| } | ||
| let nextItems = configuration.makeItems( | ||
| outboundFocusSpacerHeight: activeOutboundFocusRowID.map { _ in | ||
| outboundFocusSpacerHeight(in: tableView) | ||
| } | ||
| ) | ||
| let shouldReload = nextItems != items | ||
| || configuration.expandedIDs != expandedIDs | ||
| || configuration.agentState != agentState | ||
| let shouldScrollToBottom = scrollToBottomRequest != lastScrollToBottomRequest | ||
| lastScrollToBottomRequest = scrollToBottomRequest | ||
| let wasAtBottom = isAtBottom.wrappedValue | ||
| || distanceFromBottom(in: tableView) <= chatTranscriptAtBottomThreshold | ||
| let anchor = firstVisibleAnchor(in: tableView) | ||
|
|
||
| guard shouldReload else { | ||
| if shouldScrollToBottom { | ||
| scrollToBottom(in: tableView, animated: true) | ||
| } else if shouldFocusOutbound, let outboundFocusRowID { | ||
| focusOutbound(rowID: outboundFocusRowID, in: tableView, animated: true) | ||
| } | ||
| updateBottomState(from: tableView) | ||
| return | ||
|
|
@@ -139,7 +159,9 @@ struct ChatTranscriptTableView: UIViewRepresentable { | |
| tableView.reloadData() | ||
| tableView.layoutIfNeeded() | ||
|
|
||
| if shouldScrollToBottom || wasAtBottom { | ||
| if shouldFocusOutbound, let outboundFocusRowID { | ||
| focusOutbound(rowID: outboundFocusRowID, in: tableView, animated: false) | ||
| } else if shouldScrollToBottom || wasAtBottom { | ||
| scrollToBottom(in: tableView, animated: false) | ||
| } else if let anchor { | ||
| restore(anchor, in: tableView) | ||
|
|
@@ -170,6 +192,34 @@ struct ChatTranscriptTableView: UIViewRepresentable { | |
| return cell | ||
| } | ||
|
|
||
| func tableView(_ tableView: UITableView, heightForRowAt indexPath: IndexPath) -> CGFloat { | ||
| guard items.indices.contains(indexPath.row) else { | ||
| return UITableView.automaticDimension | ||
| } | ||
| switch items[indexPath.row] { | ||
| case .bottomAnchor: | ||
| return 9 | ||
| case .outboundFocusSpacer(let height): | ||
| return height | ||
| default: | ||
| return UITableView.automaticDimension | ||
| } | ||
| } | ||
|
|
||
| func tableView(_ tableView: UITableView, estimatedHeightForRowAt indexPath: IndexPath) -> CGFloat { | ||
| guard items.indices.contains(indexPath.row) else { | ||
| return tableView.estimatedRowHeight | ||
| } | ||
| switch items[indexPath.row] { | ||
| case .bottomAnchor: | ||
| return 9 | ||
| case .outboundFocusSpacer(let height): | ||
| return height | ||
| default: | ||
| return tableView.estimatedRowHeight | ||
| } | ||
| } | ||
|
|
||
| func scrollViewDidScroll(_ scrollView: UIScrollView) { | ||
| guard let tableView = scrollView as? UITableView else { return } | ||
| updateBottomState(from: tableView) | ||
|
|
@@ -198,11 +248,16 @@ struct ChatTranscriptTableView: UIViewRepresentable { | |
| defer { isHandlingLayout = false } | ||
|
|
||
| if tableView.isViewportInsetsExternallyDriven { | ||
| if let activeOutboundFocusRowID { | ||
| focusOutbound(rowID: activeOutboundFocusRowID, in: tableView, animated: false) | ||
| } | ||
| updateBottomState(from: tableView) | ||
| return | ||
| } | ||
|
|
||
| if boundsChanged, let oldViewport { | ||
| if let activeOutboundFocusRowID { | ||
| focusOutbound(rowID: activeOutboundFocusRowID, in: tableView, animated: false) | ||
| } else if boundsChanged, let oldViewport { | ||
| restoreKeyboardViewport(snapshot: oldViewport, in: tableView) | ||
| } else if isAtBottom.wrappedValue { | ||
| scrollToBottom(in: tableView, animated: false) | ||
|
|
@@ -240,6 +295,30 @@ struct ChatTranscriptTableView: UIViewRepresentable { | |
| setAtBottom(true) | ||
| } | ||
|
|
||
| private func focusOutbound(rowID: String, in tableView: UITableView, animated: Bool) { | ||
| guard let row = items.firstIndex(where: { $0.id == rowID }) else { return } | ||
| tableView.layoutIfNeeded() | ||
| let indexPath = IndexPath(row: row, section: 0) | ||
| let rect = tableView.rectForRow(at: indexPath) | ||
| let visibleTopPadding: CGFloat = 18 | ||
| let targetY = clampedOffsetY( | ||
| rect.minY - tableView.adjustedContentInset.top - visibleTopPadding, | ||
| in: tableView | ||
| ) | ||
| tableView.setContentOffset(CGPoint(x: tableView.contentOffset.x, y: targetY), animated: animated) | ||
| setAtBottom(false) | ||
| } | ||
|
|
||
| private func outboundFocusSpacerHeight(in tableView: UITableView) -> CGFloat { | ||
| let viewportHeight = max( | ||
| 0, | ||
| tableView.bounds.height | ||
| - tableView.adjustedContentInset.top | ||
| - tableView.adjustedContentInset.bottom | ||
| ) | ||
| return max(260, viewportHeight * 1.05) | ||
| } | ||
|
|
||
| private func requestOlderHistoryIfNeeded(in tableView: UITableView) { | ||
| guard let configuration, configuration.hasMoreHistory else { | ||
| topRequestKey = nil | ||
|
|
@@ -340,7 +419,7 @@ private struct ChatTranscriptTableConfiguration { | |
| let markdownRenderer: ChatMarkdownRenderer? | ||
| let contentCache: ChatContentCache? | ||
|
|
||
| func makeItems() -> [ChatTranscriptTableItem] { | ||
| func makeItems(outboundFocusSpacerHeight: CGFloat? = nil) -> [ChatTranscriptTableItem] { | ||
| var items: [ChatTranscriptTableItem] = [] | ||
| if hasMoreHistory { | ||
| items.append(.loadingMore) | ||
|
|
@@ -361,6 +440,9 @@ private struct ChatTranscriptTableConfiguration { | |
| items.append(.typing) | ||
| } | ||
| items.append(.bottomAnchor) | ||
| if let outboundFocusSpacerHeight { | ||
| items.append(.outboundFocusSpacer(height: outboundFocusSpacerHeight)) | ||
| } | ||
| return items | ||
| } | ||
|
|
||
|
|
@@ -444,6 +526,9 @@ private struct ChatTranscriptTableConfiguration { | |
| case .bottomAnchor: | ||
| Color.clear | ||
| .frame(height: 9) | ||
| case .outboundFocusSpacer(let height): | ||
| Color.clear | ||
| .frame(height: height) | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -457,6 +542,7 @@ private enum ChatTranscriptTableItem: Equatable { | |
| case row(ChatTranscriptRow) | ||
| case typing | ||
| case bottomAnchor | ||
| case outboundFocusSpacer(height: CGFloat) | ||
|
|
||
| var id: String { | ||
| switch self { | ||
|
|
@@ -476,6 +562,8 @@ private enum ChatTranscriptTableItem: Equatable { | |
| return "typing" | ||
| case .bottomAnchor: | ||
| return "bottom-anchor" | ||
| case .outboundFocusSpacer: | ||
| return "outbound-focus-spacer" | ||
| } | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 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, andreconcileTerminalPending(against:)can still remove the focused row without clearing/remappinglatestOutboundFocusRowID, 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
Source: Path instructions