Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .github/swift-file-length-budget.tsv
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
3669 cmuxTests/CLIGenericHookPersistenceTests.swift
3397 Sources/CmuxConfig.swift
3364 cmuxTests/TabManagerSessionSnapshotTests.swift
3053 Sources/Update/UpdateTitlebarAccessory.swift
3058 Sources/Update/UpdateTitlebarAccessory.swift
2876 cmuxTests/CMUXOpenCommandTests.swift
2875 Sources/SessionIndexView.swift
2606 Sources/KeyboardShortcutSettings.swift
Expand Down Expand Up @@ -226,6 +226,7 @@
519 Sources/CmuxConfigExecutor.swift
518 Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swift
518 Packages/macOS/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress-git-review-queue-command-deck.swift
516 Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptTableView.swift
516 Sources/TerminalImageTransfer.swift
514 Packages/macOS/CmuxSwiftRender/Sources/CmuxSwiftRender/ExpressionEvaluator.swift
514 cmuxUITests/UpdatePillUITests.swift
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,8 @@ struct ChatTranscriptTableView: UIViewRepresentable {
private var topRequestKey: String?
private var lastScrollToBottomRequest = 0
private var isHandlingLayout = false
private var isApplyingDataUpdate = false
private var pendingContentUpdateAnchor: ChatTranscriptTableAnchor?
private weak var tableView: ChatTranscriptUITableView?
private var isAtBottom: Binding<Bool>
#if DEBUG
Expand All @@ -97,13 +99,18 @@ struct ChatTranscriptTableView: UIViewRepresentable {

func attach(_ tableView: ChatTranscriptUITableView) {
self.tableView = tableView
tableView.afterLayout = { [weak self, weak tableView] oldBoundsSize, oldContentSize, oldViewport in
tableView.anchorBeforeLayout = { [weak self, weak tableView] in
guard let self, let tableView else { return nil }
return self.firstVisibleAnchor(in: tableView)
}
tableView.afterLayout = { [weak self, weak tableView] oldBoundsSize, oldContentSize, oldViewport, oldAnchor in
guard let self, let tableView else { return }
self.handleLayoutChange(
in: tableView,
oldBoundsSize: oldBoundsSize,
oldContentSize: oldContentSize,
oldViewport: oldViewport
oldViewport: oldViewport,
oldAnchor: oldAnchor
)
}
}
Expand All @@ -120,29 +127,34 @@ struct ChatTranscriptTableView: UIViewRepresentable {
|| configuration.agentState != agentState
let shouldScrollToBottom = scrollToBottomRequest != lastScrollToBottomRequest
lastScrollToBottomRequest = scrollToBottomRequest
let wasAtBottom = isAtBottom.wrappedValue
|| distanceFromBottom(in: tableView) <= chatTranscriptAtBottomThreshold
let wasAtBottom = distanceFromBottom(in: tableView) <= chatTranscriptAtBottomThreshold
let anchor = firstVisibleAnchor(in: tableView)

guard shouldReload else {
if shouldScrollToBottom {
pendingContentUpdateAnchor = nil
scrollToBottom(in: tableView, animated: true)
}
updateBottomState(from: tableView)
return
}

pendingContentUpdateAnchor = nil
items = nextItems
expandedIDs = configuration.expandedIDs
agentState = configuration.agentState

isApplyingDataUpdate = true
defer { isApplyingDataUpdate = false }
tableView.reloadData()
tableView.layoutIfNeeded()

if shouldScrollToBottom || wasAtBottom {
pendingContentUpdateAnchor = nil
scrollToBottom(in: tableView, animated: false)
} else if let anchor {
restore(anchor, in: tableView)
pendingContentUpdateAnchor = anchor
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
#if DEBUG
applyDebugInitialScrollIfNeeded(in: tableView)
Expand Down Expand Up @@ -179,17 +191,25 @@ struct ChatTranscriptTableView: UIViewRepresentable {
requestOlderHistoryIfNeeded(in: tableView)
}

func scrollViewWillBeginDragging(_ scrollView: UIScrollView) {
pendingContentUpdateAnchor = nil
}

private func handleLayoutChange(
in tableView: ChatTranscriptUITableView,
oldBoundsSize: CGSize,
oldContentSize: CGSize,
oldViewport: MobileScrollViewportSnapshot?
oldViewport: MobileScrollViewportSnapshot?,
oldAnchor: ChatTranscriptTableAnchor?
) {
guard !isHandlingLayout else { return }
let boundsChanged = abs(oldBoundsSize.height - tableView.bounds.height) > 0.5
|| abs(oldBoundsSize.width - tableView.bounds.width) > 0.5
let contentChanged = abs(oldContentSize.height - tableView.contentSize.height) > 0.5
guard boundsChanged || contentChanged else {
if !isApplyingDataUpdate {
pendingContentUpdateAnchor = nil
}
updateBottomState(from: tableView)
return
}
Expand All @@ -201,11 +221,20 @@ struct ChatTranscriptTableView: UIViewRepresentable {
updateBottomState(from: tableView)
return
}
if isApplyingDataUpdate {
updateBottomState(from: tableView)
return
}

if boundsChanged, let oldViewport {
restoreKeyboardViewport(snapshot: oldViewport, in: tableView)
} else if isAtBottom.wrappedValue {
} else if contentChanged, let pendingContentUpdateAnchor {
restore(pendingContentUpdateAnchor, in: tableView)
self.pendingContentUpdateAnchor = nil
} else if oldViewport?.wasAtBottom == true {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Potential gap between scrollToBottom() and lastViewport update

When update() calls scrollToBottom() (because wasAtBottom was true), it calls tableView.setContentOffset(…, animated: false). UIKit marks the table as needing layout (setNeedsLayout) but does not call layoutSubviews synchronously — recordViewport() is therefore not called until the next run-loop pass. If a SwiftUI self-sizing layout fires in between (before that deferred layout), oldViewport passed here still holds the pre-scrollToBottom snapshot with wasAtBottom == false, and the branch falls through to restore(oldAnchor) instead. The anchor captured at the bottom will keep the first-visible row in place, but newly added rows that landed below the viewport remain hidden — the "stay pinned to bottom during live output" invariant is silently broken.

The previous code used isAtBottom.wrappedValue, which is written synchronously by setAtBottom(true) inside scrollToBottom(), so it was always up-to-date for this path. One option is to call recordViewport() (or an equivalent snapshot update on lastViewport) immediately after setContentOffset inside scrollToBottom() so that lastViewport.wasAtBottom is authoritative before control returns to any callers that may trigger further layouts.

scrollToBottom(in: tableView, animated: false)
} else if contentChanged, let oldAnchor {
restore(oldAnchor, in: tableView)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
updateBottomState(from: tableView)
}
Expand All @@ -231,12 +260,14 @@ struct ChatTranscriptTableView: UIViewRepresentable {
y: clampedOffsetY(rect.minY + anchor.offsetFromRowTop, in: tableView)
)
tableView.setContentOffset(offset, animated: false)
(tableView as? ChatTranscriptUITableView)?.recordCurrentViewport()
}

private func scrollToBottom(in tableView: UITableView, animated: Bool) {
tableView.layoutIfNeeded()
let targetY = maxOffsetY(in: tableView)
tableView.setContentOffset(CGPoint(x: tableView.contentOffset.x, y: targetY), animated: animated)
(tableView as? ChatTranscriptUITableView)?.recordCurrentViewport()
setAtBottom(true)
}

Expand Down Expand Up @@ -302,6 +333,7 @@ struct ChatTranscriptTableView: UIViewRepresentable {
let maxY = maxOffsetY(in: tableView)
let targetY = clampedOffsetY(minY + ((maxY - minY) * 0.5), in: tableView)
tableView.setContentOffset(CGPoint(x: tableView.contentOffset.x, y: targetY), animated: false)
(tableView as? ChatTranscriptUITableView)?.recordCurrentViewport()
setAtBottom(false)
}
#endif
Expand All @@ -320,6 +352,7 @@ struct ChatTranscriptTableView: UIViewRepresentable {
CGPoint(x: tableView.contentOffset.x, y: offsetY),
animated: false
)
(tableView as? ChatTranscriptUITableView)?.recordCurrentViewport()
setAtBottom(snapshot.wasAtBottom)
}
}
Expand Down Expand Up @@ -480,9 +513,4 @@ private enum ChatTranscriptTableItem: Equatable {
}
}

private struct ChatTranscriptTableAnchor {
let id: String
let offsetFromRowTop: CGFloat
}

#endif
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@ final class ChatTranscriptUITableView: UITableView {
var afterLayout: ((
_ oldBoundsSize: CGSize,
_ oldContentSize: CGSize,
_ oldViewport: MobileScrollViewportSnapshot?
_ oldViewport: MobileScrollViewportSnapshot?,
_ oldAnchor: ChatTranscriptTableAnchor?
) -> Void)?
var anchorBeforeLayout: (() -> ChatTranscriptTableAnchor?)?
#if DEBUG
var keyboardDebugEventCount = 0
var keyboardDebugOverlap: CGFloat = 0
Expand Down Expand Up @@ -44,18 +46,23 @@ final class ChatTranscriptUITableView: UITableView {
}
#endif

override var contentOffset: CGPoint {
didSet { recordViewport() }
}

override func layoutSubviews() {
let oldBoundsSize = lastBoundsSize
let oldContentSize = lastContentSize
let oldViewport = lastViewport
let oldAnchor = anchorBeforeLayout?()
super.layoutSubviews()
lastBoundsSize = bounds.size
lastContentSize = contentSize
recordViewport()
#if DEBUG
updateDebugAccessibilityValue()
#endif
afterLayout?(oldBoundsSize, oldContentSize, oldViewport)
afterLayout?(oldBoundsSize, oldContentSize, oldViewport, oldAnchor)
}

func keyboardViewportSnapshot() -> MobileScrollViewportSnapshot {
Expand All @@ -72,6 +79,13 @@ final class ChatTranscriptUITableView: UITableView {
restoreKeyboardViewport(snapshot, boundsHeight: bounds.height)
}

func recordCurrentViewport() {
recordViewport()
#if DEBUG
updateDebugAccessibilityValue()
#endif
}

func restoreKeyboardViewport(
_ snapshot: MobileScrollViewportSnapshot,
boundsHeight: CGFloat
Expand All @@ -83,10 +97,7 @@ final class ChatTranscriptUITableView: UITableView {
adjustedBottomInset: adjustedContentInset.bottom
)
setContentOffset(CGPoint(x: contentOffset.x, y: targetY), animated: false)
recordViewport()
#if DEBUG
updateDebugAccessibilityValue()
#endif
recordCurrentViewport()
}

func applyTranscriptViewportInsets(
Expand Down Expand Up @@ -254,4 +265,10 @@ final class ChatTranscriptUITableView: UITableView {
}
#endif
}

struct ChatTranscriptTableAnchor {
let id: String
let offsetFromRowTop: CGFloat
}

#endif
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ public struct ChatTerminalCardView: View {
.contentShape(.rect)
}
.buttonStyle(.plain)
.accessibilityIdentifier("ChatTerminalToggle-\(rowID)")
.accessibilityLabel(headerAccessibilityLabel)
.accessibilityValue(
isExpanded
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ public struct ChatToolUseRowView: View {
.contentShape(.rect)
}
.buttonStyle(.plain)
.accessibilityIdentifier("ChatToolUseToggle-\(rowID)")
.accessibilityValue(
isExpanded
? String(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,28 +73,7 @@ public struct TerminalCommandBlockView: View {
.frame(width: 2.5)
}
}
.accessibilityElement(children: .combine)
.accessibilityLabel(accessibilityLabel)
// `.combine` absorbs the inline "more lines" button, so expose the
// toggle as a VoiceOver custom action when the output is collapsible.
.accessibilityActions {
if lines.count > Self.collapseThreshold {
Button(
isExpanded
? String(
localized: "chat.terminal.collapse.action",
defaultValue: "Show less output",
bundle: .module
)
: String(
localized: "chat.terminal.expand.action",
defaultValue: "Show all output",
bundle: .module
),
action: onToggleExpanded
)
}
}
.accessibilityIdentifier("TerminalCommandBlock-\(block.id)")
}

private var commandRow: some View {
Expand All @@ -110,22 +89,23 @@ public struct TerminalCommandBlockView: View {
}
}

@ViewBuilder
private func outputBlock(_ lines: [String]) -> some View {
ScrollView(.horizontal, showsIndicators: false) {
VStack(alignment: .leading, spacing: 0) {
if !isExpanded, lines.count > Self.collapseThreshold {
collapsedOutput(lines)
} else {
outputText(lines)
}
if !isExpanded, lines.count > Self.collapseThreshold {
collapsedOutput(lines)
} else {
ScrollView(.horizontal, showsIndicators: false) {
outputText(lines)
}
}
}

@ViewBuilder
private func collapsedOutput(_ lines: [String]) -> some View {
let hidden = lines.count - Self.collapsedHeadCount - Self.collapsedTailCount
outputText(Array(lines.prefix(Self.collapsedHeadCount)))
ScrollView(.horizontal, showsIndicators: false) {
outputText(Array(lines.prefix(Self.collapsedHeadCount)))
}
Button(action: onToggleExpanded) {
Text(
String(
Expand All @@ -136,11 +116,23 @@ public struct TerminalCommandBlockView: View {
)
.font(.system(size: 12, design: .monospaced))
.foregroundStyle(theme.accent)
.padding(.vertical, 1)
.padding(.vertical, 4)
.frame(minHeight: 28, alignment: .leading)
.contentShape(Rectangle())
}
.buttonStyle(.plain)
outputText(Array(lines.suffix(Self.collapsedTailCount)))
.opacity(0.55)
.accessibilityElement(children: .combine)
.accessibilityLabel(
isExpanded
? String(localized: "chat.terminal.collapse.action", defaultValue: "Show less output", bundle: .module)
: String(localized: "chat.terminal.expand.action", defaultValue: "Show all output", bundle: .module)
)
.accessibilityIdentifier("TerminalCommandBlockToggle-\(block.id)")
.accessibilityAddTraits(.isButton)
ScrollView(.horizontal, showsIndicators: false) {
outputText(Array(lines.suffix(Self.collapsedTailCount)))
.opacity(0.55)
}
}

private func outputText(_ lines: [String]) -> some View {
Expand Down
Loading
Loading