Skip to content
Closed
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
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,16 @@ struct TerminalTextSheetView: View {
/// workspace has no terminal; the sheet then shows its empty state.
let surfaceID: String?

/// The capture started by `openTextSheetFromMenu` the instant the menu item
/// was tapped, while the terminal surface was still fully window-attached.
/// Preferred over re-resolving from the registry inside `.task`: by the time
/// the sheet's `.task` runs the presenter's window/alpha may have dropped,
/// and the registry pick is visibility-scoped, so a late resolve could miss
/// the live surface and show the empty state. The sheet just awaits this.
/// Nil falls back to a fresh resolve so the path still works if the capture
/// was never armed.
let capture: Task<String?, Never>?

@Environment(\.dismiss) private var dismiss

/// Loaded once per presentation in `.task`; nil while the off-main surface
Expand Down Expand Up @@ -107,11 +117,20 @@ struct TerminalTextSheetView: View {
}

private func loadSnapshot() async {
guard let surfaceID else {
isLoading = false
return
// Prefer the capture armed at tap time (surface still fully on screen).
// Only fall back to a fresh resolve when no capture was provided, which
// re-resolves from the registry and can miss the live surface if the
// sheet's presentation dropped the presenter's window/alpha.
let fullText: String?
if let capture {
fullText = await capture.value
} else {
guard let surfaceID else {
isLoading = false
return
}
fullText = await GhosttySurfaceView.copyableTerminalText(surfaceID: surfaceID)
}
let fullText = await GhosttySurfaceView.copyableTerminalText(surfaceID: surfaceID)
// Cap off the main actor: the capture is bounded by the iOS surface's
// scrollback-limit (~2MB, see applyiOSDefaults), but splitting and
// rejoining even that much text is O(content) string work that would
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,15 @@ struct WorkspaceDetailView: View {
/// workspace selection changes underneath it (e.g. Mac-side sync) while
/// the sheet is open; the sheet loads its snapshot once per presentation.
@State private var textSheetSurfaceID: String?
/// The "View as Text" capture, kicked off the instant the menu item is
/// tapped — while the terminal surface is still fully window-attached and
/// visible — rather than from the sheet's own `.task`. Presenting a sheet
/// can briefly drop the presenter's window/alpha, and the registry pick is
/// visibility-scoped, so re-resolving the surface after presentation could
/// miss the one live surface and show the empty state. Resolving at tap time
/// captures the surface (and FIFO-orders its queued read) before any of that
/// can happen; the sheet just awaits the result.
@State private var textSheetCapture: Task<String?, Never>?
/// Chat-mode toggle: when on (and a session exists) the detail renders
/// the agent chat inline in place of the terminal. The toolbar button
/// flips this; there is no cover and no Done button.
Expand Down Expand Up @@ -441,8 +450,8 @@ struct WorkspaceDetailView: View {
.sheet(isPresented: $isFeedbackComposerPresented) {
feedbackComposer
}
.sheet(isPresented: $isTextSheetPresented) {
TerminalTextSheetView(surfaceID: textSheetSurfaceID)
.sheet(isPresented: $isTextSheetPresented, onDismiss: { textSheetCapture = nil }) {

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 The onDismiss callback releases the task reference but does not call cancel() on it. If the user dismisses the sheet before the outputQueue read completes, the unstructured Task continues running until it finishes (the surface read acquires the surface lock), and capture.value in the sheet's already-cancelled .task is simply dropped. For a user-triggered, bounded read this is harmless, but explicitly cancelling is the safer idiom and signals intent to cooperating callers.

Suggested change
.sheet(isPresented: $isTextSheetPresented, onDismiss: { textSheetCapture = nil }) {
.sheet(isPresented: $isTextSheetPresented, onDismiss: {
textSheetCapture?.cancel()
textSheetCapture = nil
}) {

TerminalTextSheetView(surfaceID: textSheetSurfaceID, capture: textSheetCapture)
}
.workspaceRenameDialog(
isPresented: $isRenamePresented,
Expand Down Expand Up @@ -649,7 +658,16 @@ struct WorkspaceDetailView: View {
/// Opens the "View as Text" sheet: the terminal's content as selectable
/// plain text, because the render surface itself has no copy affordance.
private func openTextSheetFromMenu() {
textSheetSurfaceID = selectedTerminal?.id.rawValue
let surfaceID = selectedTerminal?.id.rawValue
textSheetSurfaceID = surfaceID
// Resolve + read NOW, on the main actor, while the terminal surface is
// still fully on screen. `copyableTerminalText` does its visibility-scoped
// registry pick and FIFO-enqueues the off-main read synchronously before
// returning the awaitable, so the surface can never be filtered out by a
// window/alpha drop that the sheet's own presentation would cause.
textSheetCapture = surfaceID.map { id in
Task { await GhosttySurfaceView.copyableTerminalText(surfaceID: id) }
}
isTextSheetPresented = true
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import CmuxMobileDiagnostics
import CmuxMobileTerminalKit
import GhosttyKit
import UIKit

Expand Down Expand Up @@ -85,23 +87,60 @@ extension GhosttySurfaceView {
// sheet's honest empty state over silently copying that stale text.
// If the same terminal is mounted in several scenes the contents are
// identical, so the lowest-keyed visible match keeps the pick
// deterministic.
let matchingView = registeredSurfaceViews
// deterministic. The eligibility rule itself lives in
// `CopyableTerminalTextSelection` so it is host-testable without UIKit;
// this just maps the live registry onto that pure predicate.
//
// The resolve happens on the main actor at the call site (the menu tap),
// BEFORE the sheet finishes presenting, so the surface is still
// window-attached and visible when the predicate runs — a sheet
// presentation that briefly drops the presenter's window/alpha can no
// longer turn the live surface into a miss.
let orderedViews = registeredSurfaceViews
.sorted { $0.key < $1.key }
.compactMap(\.value.value)
.first { candidate in
candidate.hostSurfaceID == surfaceID && candidate.surface != nil
&& candidate.window != nil && !candidate.isHidden
&& candidate.alpha > 0.01
}
guard let surface = matchingView?.surface else { return nil }
let candidates = orderedViews.map { view in
CopyableTerminalTextSelection.Candidate(
hostSurfaceID: view.hostSurfaceID,
hasSurface: view.surface != nil,
hasWindow: view.window != nil,
isHidden: view.isHidden,
alpha: Double(view.alpha)
)
}
let chosen = CopyableTerminalTextSelection.chosenIndex(from: candidates, for: surfaceID)
.map { orderedViews[$0] }

#if DEBUG
let candidateSummary = candidates
.map { "[id=\($0.hostSurfaceID ?? "nil") surf=\($0.hasSurface) win=\($0.hasWindow) hidden=\($0.isHidden) a=\(String(format: "%.2f", $0.alpha))]" }
.joined(separator: " ")
MobileDebugLog.anchormux(
"viewAsText.pick want=\(surfaceID) count=\(candidates.count) chosen=\(chosen == nil ? "none" : "yes") \(candidateSummary)"
)
#endif

guard let surface = chosen?.surface else { return nil }
let handle = CopyableTextSurfaceHandle(surface: surface)
return await withCheckedContinuation { continuation in
outputQueue.async {
// SCREEN = scrollback + all written rows. Fall back to the
// viewport-only read if the screen read fails outright.
let text = surfaceText(handle.surface, pointTag: GHOSTTY_POINT_SCREEN)
?? surfaceText(handle.surface, pointTag: GHOSTTY_POINT_VIEWPORT)
// SCREEN = scrollback + all written rows. `surfaceText` returns a
// non-nil empty string for a zero-byte range, so a plain `??`
// never fell back when SCREEN read empty-but-ok. Route both reads
// through the pure decision so a nil OR empty SCREEN still tries
// VIEWPORT before the sheet gives up.
let screen = surfaceText(handle.surface, pointTag: GHOSTTY_POINT_SCREEN)
let viewport = surfaceText(handle.surface, pointTag: GHOSTTY_POINT_VIEWPORT)
let text = CopyableTerminalTextSelection.resolvedText(screen: screen, viewport: viewport)
Comment on lines +132 to +134

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 Both the SCREEN and VIEWPORT reads are issued eagerly even when SCREEN has content. The old code's lazy ?? chain only read VIEWPORT when SCREEN returned nil. The problem the PR fixes (empty SCREEN never falling back) could be addressed without reading VIEWPORT unconditionally: read VIEWPORT only when SCREEN is absent or empty. This avoids a second ghostty_surface_read_text call (and the lock acquisition it implies) on the common case where SCREEN succeeds with content.

Suggested change
let screen = surfaceText(handle.surface, pointTag: GHOSTTY_POINT_SCREEN)
let viewport = surfaceText(handle.surface, pointTag: GHOSTTY_POINT_VIEWPORT)
let text = CopyableTerminalTextSelection.resolvedText(screen: screen, viewport: viewport)
let screen = surfaceText(handle.surface, pointTag: GHOSTTY_POINT_SCREEN)
let viewport: String? = (screen?.isEmpty ?? true)
? surfaceText(handle.surface, pointTag: GHOSTTY_POINT_VIEWPORT)
: nil
let text = CopyableTerminalTextSelection.resolvedText(screen: screen, viewport: viewport)

#if DEBUG
func describe(_ value: String?) -> String {
guard let value else { return "nil" }
return value.isEmpty ? "empty" : "\(value.count)chars"
}
MobileDebugLog.anchormux(
"viewAsText.read screen=\(describe(screen)) viewport=\(describe(viewport)) resolved=\(describe(text))"
)
#endif
continuation.resume(returning: text)
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
import Foundation

/// Pure selection + fallback logic for the iOS "View as Text" capture, split
/// out of `GhosttySurfaceRegistry` so the two decisions that actually decide
/// whether the sheet shows text or its empty state are host-testable without
/// libghostty, UIKit, or a live surface.
///
/// Two things were silently making the sheet show "No terminal text available"
/// even with visible output:
///
/// 1. The candidate predicate is id-scoped AND visibility-scoped. If the chosen
/// surface is re-resolved from the registry *after* the sheet has begun
/// presenting, a transient `window == nil` / `isHidden` / `alpha` drop on the
/// presenter can exclude the one live surface, yielding nil. The fix moves the
/// resolve to tap time (see `GhosttySurfaceRegistry`), but the predicate is
/// still the single source of truth and is unit-tested here.
/// 2. `surfaceText` returns a non-nil empty string when the range has zero bytes,
/// so `screen ?? viewport` never fell back when SCREEN read empty-but-ok. The
/// `resolvedText` decision below treats nil OR empty SCREEN as "try VIEWPORT".
public enum CopyableTerminalTextSelection {

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 Caseless enum used as static-function namespace

CopyableTerminalTextSelection is a caseless enum whose entire API is three public static funcs — this is the namespace-enum antipattern flagged by the cmux no-ambient-global-state rule. The comment block in the rule explicitly calls out "a caseless enum or empty struct used purely as a static func/static let namespace." The fix that keeps it host-testable without UIKit is top-level free functions in CmuxMobileTerminalKit (pass for private/fileprivate file-scope pure helpers) or a concrete struct with an initializer — either form lets the test suite reach them via @testable import without requiring a static-namespace enum.

Rule Used: Flag new ambient global state in production Swift:... (source)

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!

/// Inputs describing a registered surface view for the eligibility decision.
/// Mirrors the runtime fields read off `GhosttySurfaceView` (`hostSurfaceID`,
/// `surface != nil`, `window != nil`, `isHidden`, `alpha`) as plain values so
/// the rule is testable without a UIKit view.
public struct Candidate: Equatable, Sendable {
public let hostSurfaceID: String?
public let hasSurface: Bool
public let hasWindow: Bool
public let isHidden: Bool
public let alpha: Double

public init(
hostSurfaceID: String?,
hasSurface: Bool,
hasWindow: Bool,
isHidden: Bool,
alpha: Double
) {
self.hostSurfaceID = hostSurfaceID
self.hasSurface = hasSurface
self.hasWindow = hasWindow
self.isHidden = isHidden
self.alpha = alpha
}
}

/// Whether `candidate` is the live, on-screen surface for `surfaceID` and so
/// may back the "View as Text" capture.
///
/// The id scoping keeps a second visible surface (another iPad scene, an
/// in-flight transition) from leaking a different workspace's terminal into
/// the capture; `hasSurface` keeps a dismantling view (registry-resident with
/// a non-nil pointer until its queued dispose runs) from contributing stale
/// text; the window/hidden/alpha gates keep an off-screen surface out.
public static func isEligible(_ candidate: Candidate, for surfaceID: String) -> Bool {
candidate.hostSurfaceID == surfaceID
&& candidate.hasSurface
&& candidate.hasWindow
&& !candidate.isHidden
&& candidate.alpha > 0.01
}

/// The deterministic pick from the registered candidates: the lowest-keyed
/// eligible match. When the same terminal is mounted in several scenes the
/// contents are identical, so the lowest-keyed visible match keeps the pick
/// stable. `candidates` must be supplied lowest-key-first.
///
/// - Returns: The index of the chosen candidate in `candidates`, or nil when
/// none is eligible.
public static func chosenIndex(
from candidates: [Candidate],
for surfaceID: String
) -> Int? {
candidates.firstIndex { isEligible($0, for: surfaceID) }
}

/// The text the sheet should show given the SCREEN and VIEWPORT reads.
///
/// `surfaceText` returns nil on a failed read and a non-nil empty string for
/// a zero-byte range, so a plain `screen ?? viewport` never fell back when
/// SCREEN read empty-but-ok. Here a SCREEN result that is nil OR empty falls
/// through to VIEWPORT, and only a VIEWPORT result that is also nil/empty
/// yields nil (the honest empty state).
///
/// - Parameters:
/// - screen: The SCREEN (scrollback + all rows) read, nil on failure.
/// - viewport: The VIEWPORT (visible rows) read, nil on failure.
/// - Returns: The non-empty text to show, or nil when neither has content.
public static func resolvedText(screen: String?, viewport: String?) -> String? {
if let screen, !screen.isEmpty { return screen }
if let viewport, !viewport.isEmpty { return viewport }
return nil
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
import Foundation
import Testing
@testable import CmuxMobileTerminalKit

@Suite("CopyableTerminalTextSelection: View as Text pick + fallback")
struct CopyableTerminalTextSelectionTests {
typealias Candidate = CopyableTerminalTextSelection.Candidate

private func visible(_ id: String?) -> Candidate {
Candidate(hostSurfaceID: id, hasSurface: true, hasWindow: true, isHidden: false, alpha: 1)
}

@Test("id-scoped, on-screen surface is eligible")
func eligibleMatch() {
#expect(CopyableTerminalTextSelection.isEligible(visible("surface:1"), for: "surface:1"))
}

@Test("a different id is never eligible")
func excludedByID() {
#expect(!CopyableTerminalTextSelection.isEligible(visible("surface:2"), for: "surface:1"))
#expect(!CopyableTerminalTextSelection.isEligible(visible(nil), for: "surface:1"))
}

@Test("a dismantling view with no surface is excluded")
func excludedWhenNoSurface() {
let dismantling = Candidate(
hostSurfaceID: "surface:1", hasSurface: false, hasWindow: true, isHidden: false, alpha: 1
)
#expect(!CopyableTerminalTextSelection.isEligible(dismantling, for: "surface:1"))
}

@Test("off-window / hidden / transparent surfaces are excluded")
func excludedWhenOffScreen() {
let detached = Candidate(
hostSurfaceID: "surface:1", hasSurface: true, hasWindow: false, isHidden: false, alpha: 1
)
let hidden = Candidate(
hostSurfaceID: "surface:1", hasSurface: true, hasWindow: true, isHidden: true, alpha: 1
)
let transparent = Candidate(
hostSurfaceID: "surface:1", hasSurface: true, hasWindow: true, isHidden: false, alpha: 0
)
#expect(!CopyableTerminalTextSelection.isEligible(detached, for: "surface:1"))
#expect(!CopyableTerminalTextSelection.isEligible(hidden, for: "surface:1"))
#expect(!CopyableTerminalTextSelection.isEligible(transparent, for: "surface:1"))
}

@Test("chosenIndex returns the first (lowest-keyed) eligible match")
func chosenIndexDeterministic() {
let candidates = [
visible("surface:2"), // wrong id
visible("surface:1"), // first eligible
visible("surface:1"), // also eligible, but later
]
#expect(CopyableTerminalTextSelection.chosenIndex(from: candidates, for: "surface:1") == 1)
}

@Test("chosenIndex is nil when nothing is eligible")
func chosenIndexNone() {
let candidates = [visible("surface:2"), visible(nil)]
#expect(CopyableTerminalTextSelection.chosenIndex(from: candidates, for: "surface:1") == nil)
}

@Test("non-empty SCREEN wins")
func screenWins() {
#expect(
CopyableTerminalTextSelection.resolvedText(screen: "scrollback", viewport: "visible")
== "scrollback"
)
}

@Test("empty-string SCREEN falls back to VIEWPORT")
func emptyScreenFallsBackToViewport() {
// The exact reported bug: SCREEN read OK but zero-byte (non-nil ""), so a
// plain `screen ?? viewport` would keep the empty string and the sheet
// showed its empty state even though the viewport had content.
#expect(
CopyableTerminalTextSelection.resolvedText(screen: "", viewport: "visible text")
== "visible text"
)
}

@Test("nil SCREEN falls back to VIEWPORT")
func nilScreenFallsBackToViewport() {
#expect(
CopyableTerminalTextSelection.resolvedText(screen: nil, viewport: "visible text")
== "visible text"
)
}

@Test("both empty/nil yields nil (honest empty state)")
func bothEmptyYieldsNil() {
#expect(CopyableTerminalTextSelection.resolvedText(screen: "", viewport: "") == nil)
#expect(CopyableTerminalTextSelection.resolvedText(screen: nil, viewport: nil) == nil)
#expect(CopyableTerminalTextSelection.resolvedText(screen: "", viewport: nil) == nil)
}
}
Loading