Repository navigation
iOS: make terminal folder-path taps a setting (Open Folders on Tap) - #8524
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a persisted “Open Folders on Tap” setting, routes it through terminal surfaces, authorizes artifact metadata lookup, uses artifact kind to choose between opening artifacts and focusing the terminal, and strips terminal escape sequences before path extraction. ChangesTerminal folder tap behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant GhosttySurfaceCoordinator
participant TerminalFolderTapPolicy
participant terminalArtifactStat
participant TerminalArtifactReadContext
User->>GhosttySurfaceCoordinator: Tap artifact path
GhosttySurfaceCoordinator->>TerminalFolderTapPolicy: Evaluate tap policy
TerminalFolderTapPolicy->>terminalArtifactStat: Request artifact kind when disabled
terminalArtifactStat->>TerminalArtifactReadContext: Authorize stat path
TerminalArtifactReadContext-->>terminalArtifactStat: Allow canonical file or directory-list path
terminalArtifactStat-->>TerminalFolderTapPolicy: Return artifact metadata
alt Focus terminal
TerminalFolderTapPolicy-->>GhosttySurfaceCoordinator: focusTerminal
GhosttySurfaceCoordinator->>GhosttySurfaceCoordinator: clickTerminal
else Open artifact
TerminalFolderTapPolicy-->>GhosttySurfaceCoordinator: openArtifact
GhosttySurfaceCoordinator->>GhosttySurfaceCoordinator: onArtifactPathTapped
end
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (22 passed)
✨ Finishing Touches🧪 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 adds an "Open Folders on Tap" setting for iOS that lets users suppress accidental folder-browser openings when tapping directory names in terminal output. When the setting is off, a bounded (2 s) Mac-side
Confidence Score: 5/5Safe to merge. The new tap-classification path is well-guarded by generation counters, surfaceView identity checks, and a 2-second deadline that fails closed on infrastructure errors. All three bug fixes are behaviorally correct and test-covered. The setting plumbing, tap-policy logic, escape-sequence stripping, and Mac-side directory-authorization fix are all narrowly scoped and carry comprehensive new tests. The only open concern — thumbnail re-opening the file by URL after closing the verified fd rather than keeping the fd alive through ImageIO — is a narrow TOCTOU edge that does not regress existing behavior and is noted as a suggestion. ArtifactByteReader.swift — the thumbnail function open-verify-close-then-CGImageSourceCreateWithURL pattern leaves a narrow window; the suggestion is to keep the fd open and use CGImageSourceCreateWithData instead. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[iOS tap at col/row] --> B{artifactFilesEnabled?}
B -- No --> Z[clickTerminal + .focusTerminal]
B -- Yes --> C[visibleTextForArtifactHitTesting]
C --> D{path token at coordinates?}
D -- No --> Z
D -- Yes --> E{folderTapEnabled?}
E -- Yes --> F[revalidate path in snapshot]
E -- No --> G[TerminalFolderTapPolicy.decision race: stat vs 2s deadline]
G --> H{ChatArtifactKind}
H -- .directory --> I[revalidate path focusTerminal branch]
H -- .image/.text/.binary --> F
H -- .forbidden --> F
H -- infra error / deadline --> I
I --> J{currentPath == path?}
J -- No --> K[.ignored]
J -- Yes --> L[clickTerminal fire-and-forget Task]
L --> M[.focusTerminal]
F --> N{currentPath == path?}
N -- No --> K
N -- Yes --> O[onArtifactPathTapped .openedArtifact]
%%{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"}}}%%
flowchart TD
A[iOS tap at col/row] --> B{artifactFilesEnabled?}
B -- No --> Z[clickTerminal + .focusTerminal]
B -- Yes --> C[visibleTextForArtifactHitTesting]
C --> D{path token at coordinates?}
D -- No --> Z
D -- Yes --> E{folderTapEnabled?}
E -- Yes --> F[revalidate path in snapshot]
E -- No --> G[TerminalFolderTapPolicy.decision race: stat vs 2s deadline]
G --> H{ChatArtifactKind}
H -- .directory --> I[revalidate path focusTerminal branch]
H -- .image/.text/.binary --> F
H -- .forbidden --> F
H -- infra error / deadline --> I
I --> J{currentPath == path?}
J -- No --> K[.ignored]
J -- Yes --> L[clickTerminal fire-and-forget Task]
L --> M[.focusTerminal]
F --> N{currentPath == path?}
N -- No --> K
N -- Yes --> O[onArtifactPathTapped .openedArtifact]
Reviews (20): Last reviewed commit: "fix: reject typed special artifacts and ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| struct TerminalFolderTapPolicy: Sendable { | ||
| /// The action the terminal tap handler should take for a detected path. | ||
| enum Decision: Sendable, Equatable { | ||
| case openArtifact | ||
| case focusTerminal | ||
| } | ||
|
|
||
| /// Applies the folder-tap preference without adding a stat call while enabled. | ||
| static func decision( | ||
| for path: String, | ||
| folderTapEnabled: Bool, | ||
| stat: @MainActor @Sendable (String) async throws -> ChatArtifactKind | ||
| ) async -> Decision { | ||
| guard !folderTapEnabled else { return .openArtifact } | ||
|
|
||
| do { | ||
| return try await stat(path) == .directory ? .focusTerminal : .openArtifact | ||
| } catch { | ||
| return .openArtifact | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
TerminalFolderTapPolicy has no stored properties and exposes exactly one static method; every caller uses it as TerminalFolderTapPolicy.decision(...). This is the empty-struct-as-static-namespace pattern the cmux-no-ambient-global-state rule explicitly flags. The canonical fix is a constructable type — move folderTapEnabled into a stored property and make decision an instance method that accepts only the stat closure, so callers create TerminalFolderTapPolicy(folderTapEnabled: flag).decision(for: path, stat: ...). The tests still access it via @testable import without any seam changes, and the type becomes genuinely injectable.
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!
There was a problem hiding this comment.
Accepted and fixed in fa266fd: TerminalFolderTapPolicy is now a constructable instance with folderTapEnabled as a stored property and decision(for:stat:) as an instance method.
— Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalFolderTapPolicy.swift`:
- Around line 19-23: Update TerminalFolderTapPolicy to return .focusTerminal in
the stat(path) catch block. In
Packages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/TerminalFolderTapPolicyTests.swift
lines 65-74, rename the failure test and expect .focusTerminal. In
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceCoordinator+Artifacts.swift
lines 272-275, throw an error when the source is unavailable instead of
returning a fake .binary kind.
🪄 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: 02264650-9f46-4abb-b067-bf4f0f3ab9e4
📒 Files selected for processing (10)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceCoordinator+Artifacts.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileDisplaySettings.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileSettingsView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/Resources/Localizable.xcstringsPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalFolderTapPolicy.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView+TerminalArtifacts.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/MobileDisplaySettingsTests.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/TerminalFolderTapPolicyTests.swiftios/cmux/Resources/Localizable.xcstrings
The @Environment displaySettings property is private to WorkspaceDetailView.swift, so the terminal-artifacts extension file reads the flag through an internal wrapper, matching terminalFilesChipEnabled.
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
| let decision: Decision | ||
| do { | ||
| let kind = try await stat(path) | ||
| decision = kind == .directory ? .focusTerminal : .openArtifact | ||
| } catch { | ||
| decision = .focusTerminal | ||
| } | ||
| continuation.yield(decision) | ||
| continuation.finish() | ||
| } |
There was a problem hiding this comment.
All stat errors — including transient network failures, fileNotFound for a file that was visible but just deleted, and the 2-second deadline firing when the Mac is under load — are caught here and return .focusTerminal. The PR description explicitly states "a stat failure still opens the viewer, so file taps keep working in both modes," but the test is named "disabled fails closed when stat throws," and the inline doc says the same — there is a direct contradiction with the stated user-visible contract.
Concretely: a user who disabled folder taps and taps a .swift file reference in the terminal, while the Mac is slow to respond (stat RPC ≥ 2 s), gets .focusTerminal — the viewer never opens — even though folderTapEnabled: false was only meant to suppress directory taps. If the intent is instead to always fail closed (never open the viewer on any stat error), the "file taps keep working in both modes" claim in the PR description and the folderTapEnabled: false user promise should be updated to reflect this restriction.
There was a problem hiding this comment.
The PR description was stale — updated. Final behavior is deliberate and documented in the closeout comment: authorization refusal (forbidden) opens the viewer so file taps via chat-scope keep working; infrastructure failures (deadline/disconnect/fileNotFound-after-deletion) fail closed to a plain tap, because the viewer could not load content on a dead link either and this setting exists to remove interruptions. Both polarities were weighed across review cycles; this split is the owner decision.
— Claude Code
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
Review closeout: 6 structured-review cycles produced 15 accepted-and-fixed findings (fail-closed policy, string-control/BEL escape parsing, C1 forms, bounded classification deadline with injected clock, tap-generation ordering, surface-detach lifecycle, FIFO/O_CLOEXEC descriptor safety, denial diagnostics off-main, extension-kind regression, viewport revalidation). Final structured review: no actionable defect. Consciously accepted exceptions, documented for the record:
|
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
Closeout addendum: the final two structured-review findings target pre-existing entrypoints outside this PR's changes — the iroh artifact transfer registry (untouched by this diff; blocking FileHandle open predates it) and decode-from-descriptor depth on the thumbnail path (whose pre-PR code had no validation at all; this PR added the descriptor-verified pre-check). Both are tracked in #8581 rather than expanded into this PR. All PR-introduced findings across 7 review cycles (16 accepted) are fixed on this head. |
1 participant
On iOS, tapping the terminal hit-tests the tapped cell for a path token and opens the artifact viewer; for a directory that means the folder-browser sheet. Prompt cwd and
lsoutput make directory names easy to hit by accident, so ordinary taps kept opening it. This PR adds a user setting to turn that off, and fixes the Mac-side authorization bugs found while verifying it end to end.1. Setting: Settings > Terminal > "Open Folders on Tap" (
MobileDisplaySettings.terminalFolderTapEnabled, default on, localized en+ja). When on, behavior is unchanged and the tap path gains no stat call. When off, a hit-tested path is classified via a Mac stat bounded by a 2-second injected-clock deadline: a directory falls through as a plain terminal tap; a file opens the viewer; a terminal-scope authorization refusal (forbidden) also opens the viewer so its richer chat-session authorization decides (bare filenames, wrapped paths); any infrastructure failure (deadline, disconnect, transport error) fails closed to a plain tap with no error UI, because the viewer could not have loaded content either and the user asked not to be interrupted. Taps apply newest-first (a per-surface generation supersedes pending classifications), outcomes are revalidated against the tapped cell before acting, and a dismantled surface ignores late results.2. Fix: terminal-scope stat authorizes directories (was always
forbidden, so terminal folder taps opened a dead "Preview unavailable" sheet and could never be classified).authorizedStatfalls back to directory-list canonicalization; fetch/thumbnail stay file-authorized; list already exposed strictly more for the same paths.3. Fix: the path detector strips VT escape sequences. The authorization text comes from a VT export whose OSC prologue glued to the first visible line, so nothing on that line ever authorized. The detector now consumes CSI/OSC/DCS/SOS/PM/APC (7-bit and C1) through their real terminators in one bounded scan — BEL terminates only OSC — so hidden string-control payloads can never become authorized paths.
4. Reader hardening from review: special files (FIFOs etc.) are never opened during classification — nonblocking
O_CLOEXECopen with descriptor-levelfstatvalidation (TOCTOU-safe); extension-derived kinds never touch the filesystem, so missing gallery files keep their kinds and folder listings do no redundant metadata calls; denial diagnostics compute off the main actor.Verified live on an isolated simulator paired to a tagged Mac build (both flag states, file taps, persistence). Tests:
CmuxAgentChat354 (detector/scope/reader incl. FIFO and C1 fixtures),MobileDisplaySettings/TerminalFolderTapPolicysuites (compiled for the simulator; run in thetest-ioslane). Structured review ran 6 cycles to a clean final pass; the consciously accepted exceptions (documented in the closeout comment) are the fail-closed-on-infrastructure-failure polarity, the bounded per-tap double stat in disabled mode, relative-token cwd divergence with an attached chat session, and background drain of an abandoned RPC that ignores cancellation.Known limitation: against an older Mac build (without fixes 2-4), classification errors fail closed, so with the toggle off folder taps behave as plain taps and some file taps may require the Mac to update.