Repository navigation
iOS: fix View as Text showing empty - #6736
lawrencecchen wants to merge 1 commit into
Conversation
Two causes made the sheet show "No terminal text available" with visible output: 1. The registry pick for the capture is visibility-scoped (window/hidden/ alpha). It ran from the sheet's own .task, after presentation, when the presenter's window/alpha can transiently drop, so the one live surface could be filtered out and the read returned nil. Resolve and FIFO-enqueue the read now happen in openTextSheetFromMenu, at tap time, while the surface is still fully on screen; the sheet just awaits that capture. 2. surfaceText returns a non-nil empty string for a zero-byte range, so screen ?? viewport never fell back when SCREEN read empty-but-ok. The new resolvedText treats a nil OR empty SCREEN as "try VIEWPORT". Extract both decisions into CopyableTerminalTextSelection (pure, no UIKit/ GhosttyKit) and unit-test the id-scoped pick, the no-surface/off-screen exclusions, and the empty-SCREEN-falls-back-to-viewport rule. Add a concise DEBUG-only anchormux diagnostic recording the requested id, every candidate's state, and the SCREEN/VIEWPORT read outcomes so the path is self-explaining in the dogfood log. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 (5)
📝 WalkthroughWalkthroughA new pure ChangesView as Text tap-time capture refactor
Sequence DiagramsequenceDiagram
participant User
participant WorkspaceDetailView
participant GhosttySurfaceRegistry
participant CopyableTerminalTextSelection
participant TerminalTextSheetView
User->>WorkspaceDetailView: tap "View as Text"
WorkspaceDetailView->>GhosttySurfaceRegistry: Task { copyableTerminalText(surfaceID:) }
Note over WorkspaceDetailView: stores in textSheetCapture
GhosttySurfaceRegistry->>CopyableTerminalTextSelection: chosenIndex(candidates, surfaceID)
GhosttySurfaceRegistry->>CopyableTerminalTextSelection: resolvedText(screen, viewport)
WorkspaceDetailView->>TerminalTextSheetView: present sheet(capture: textSheetCapture)
TerminalTextSheetView->>TerminalTextSheetView: loadSnapshot() — await capture.value
TerminalTextSheetView-->>User: display captured text
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors)
✅ Passed checks (23 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 two independent bugs that caused "View as Text" to show an empty sheet on iPhone even when the terminal had visible output. The surface registry pick (which requires the view to be window-attached and opaque) was previously invoked from inside the sheet's own
Confidence Score: 4/5Safe to merge; the two bug fixes are well-reasoned and the new test suite covers the core decision points. The fix correctly identifies and addresses both root causes. The only findings are: a caseless-enum static namespace that violates the no-ambient-global-state style rule, an eager double-read of VIEWPORT even when SCREEN succeeds, and a missing task cancel on sheet dismiss. None of these affect the correctness of the shipped fix. CopyableTerminalTextSelection.swift — the caseless enum namespace structure should be revisited before the pattern spreads to other extracted-logic files. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User
participant WDV as WorkspaceDetailView
participant GSV as GhosttySurfaceView
participant OQ as outputQueue
participant Sheet as TerminalTextSheetView
User->>WDV: tap View as Text
note over WDV: surface fully window-attached
WDV->>GSV: copyableTerminalText on MainActor
GSV->>GSV: pick eligible candidate
GSV->>OQ: enqueue SCREEN and VIEWPORT reads
GSV-->>WDV: Task capture returned
WDV->>Sheet: present with capture
note over Sheet: sheet begins presenting
Sheet->>Sheet: .task loadSnapshot
Sheet->>GSV: await capture.value
OQ-->>Sheet: resolvedText result
Sheet->>Sheet: capped snapshot computed off main
Sheet-->>User: selectable terminal text shown
User->>Sheet: dismiss
Sheet->>WDV: onDismiss clears textSheetCapture
%%{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 User
participant WDV as WorkspaceDetailView
participant GSV as GhosttySurfaceView
participant OQ as outputQueue
participant Sheet as TerminalTextSheetView
User->>WDV: tap View as Text
note over WDV: surface fully window-attached
WDV->>GSV: copyableTerminalText on MainActor
GSV->>GSV: pick eligible candidate
GSV->>OQ: enqueue SCREEN and VIEWPORT reads
GSV-->>WDV: Task capture returned
WDV->>Sheet: present with capture
note over Sheet: sheet begins presenting
Sheet->>Sheet: .task loadSnapshot
Sheet->>GSV: await capture.value
OQ-->>Sheet: resolvedText result
Sheet->>Sheet: capped snapshot computed off main
Sheet-->>User: selectable terminal text shown
User->>Sheet: dismiss
Sheet->>WDV: onDismiss clears textSheetCapture
Reviews (1): Last reviewed commit: "iOS: fix View as Text showing empty when..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /// 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 { |
There was a problem hiding this comment.
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!
| 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) |
There was a problem hiding this comment.
?? 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.
| 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) |
| } | ||
| .sheet(isPresented: $isTextSheetPresented) { | ||
| TerminalTextSheetView(surfaceID: textSheetSurfaceID) | ||
| .sheet(isPresented: $isTextSheetPresented, onDismiss: { textSheetCapture = nil }) { |
There was a problem hiding this comment.
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.
| .sheet(isPresented: $isTextSheetPresented, onDismiss: { textSheetCapture = nil }) { | |
| .sheet(isPresented: $isTextSheetPresented, onDismiss: { | |
| textSheetCapture?.cancel() | |
| textSheetCapture = nil | |
| }) { |
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
On the iPhone, "View as Text" showed the empty state "No terminal text available" even when the terminal clearly had visible output. Two causes:
The capture's registry pick is visibility-scoped (it requires the surface to be window-attached, not hidden, and opaque). It ran from the sheet's own
.task, i.e. after the sheet had begun presenting. Presenting a sheet can transiently drop the presenter's window/alpha, so the one live surface could be filtered out and the read returned nil. The resolve now happens inopenTextSheetFromMenu, at tap time, while the terminal surface is still fully on screen.copyableTerminalTextdoes its visibility-scoped pick and FIFO-enqueues the off-main read synchronously before returning the awaitable, so the surface can no longer be filtered out by a presentation-induced window/alpha drop. The sheet just awaits that capture.surfaceTextreturns a non-nil empty string ("") for a zero-byte range, not nil. SosurfaceText(SCREEN) ?? surfaceText(VIEWPORT)never fell back when the SCREEN read succeeded but came back empty. The newresolvedTexttreats a nil OR empty SCREEN as "try VIEWPORT", and only yields nil when neither has content.Both decisions are extracted into
CopyableTerminalTextSelection(pure, no UIKit/GhosttyKit) so the candidate predicate and the SCREEN-vs-viewport fallback are unit-testable. A concise DEBUG-onlyanchormuxdiagnostic records the requested id, every candidate's state (id / surface / window / hidden / alpha), and the SCREEN/VIEWPORT read outcomes, so the path is self-explaining in the dogfood log.No new user-facing strings, so no localization changes.
Testing
swift testonCmuxMobileTerminalKit(host build): 103 tests pass, including the newCopyableTerminalTextSelectionsuite covering id-scoped match, excluded-when-no-surface, off-screen exclusion, deterministic lowest-keyed pick, and empty-SCREEN-falls-back-to-viewport.xcodebuild buildfor iOS Simulator on bothCmuxMobileTerminalandCmuxMobileShellUI: BUILD SUCCEEDED.Issues
Plain-text bug (no GitHub issue): iPhone "View as Text" showed "No terminal text available" even when the terminal had visible output.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Low Risk
Localized iOS copy-sheet bugfix with preserved fallback path; no auth, networking, or data-model changes.
Overview
Fixes iPhone View as Text incorrectly showing No terminal text available when the terminal clearly had visible output.
Timing: Terminal text capture now starts in
openTextSheetFromMenuat menu tap (while the surface is still window-attached), and the sheet awaits that pre-startedTaskinstead of callingcopyableTerminalTextfrom.taskafter presentation. Sheet presentation can briefly drop the presenter’s window/alpha; the visibility-scoped registry pick used to run too late and filter out the live surface.Read fallback: SCREEN reads that succeed with
""no longer block VIEWPORT fallback (screen ?? viewporttreated empty SCREEN as success).CopyableTerminalTextSelection.resolvedTexttreats nil or empty SCREEN as “try VIEWPORT.”Testability: Surface eligibility and SCREEN/VIEWPORT resolution move into pure
CopyableTerminalTextSelectioninCmuxMobileTerminalKit, wired fromGhosttySurfaceRegistry, with unit tests. DEBUGanchormuxlogs record pick candidates and read outcomes.Reviewed by Cursor Bugbot for commit 23d3fea. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes iPhone “View as Text” showing empty when the terminal has visible output. The capture now starts at tap time and empty SCREEN reads fall back to VIEWPORT, so text reliably loads.
Bug Fixes
openTextSheetFromMenuand pass it toTerminalTextSheetView; avoids losing the visibility-scoped pick during sheet presentation.CopyableTerminalTextSelection.resolvedText; only show empty when both SCREEN and VIEWPORT have no content.Refactors
CopyableTerminalTextSelectioninCmuxMobileTerminalKitwith unit tests; integrated intoGhosttySurfaceViewinCmuxMobileTerminal.anchormuxlogs for candidate states and read outcomes.Written for commit 23d3fea. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes