Repository navigation
iOS: re-implement still-needed terminal/notification fixes from #5259 - #5519
Merged
Merged
Conversation
#5259 was 569 commits behind and edited files the iOS refactor deleted, so it was closed and the still-relevant fixes re-done fresh against current main: - Auth: run the sign-out teardown hook (push-token DELETE) BEFORE revoking the Stack session, so the server-side device-token delete can still authenticate; otherwise the device keeps receiving pushes for a signed-out account. (+test) - iOS push notification toggle strings localized (en + ja). - Notifications toggle: mirror MobilePushCoordinator.isEnabled into @State so the label/icon refresh after the async enable/disable (isEnabled is a non-observable UserDefaults read). - createTerminal(in:): target an explicit workspace so an in-flight create can't land in a drifted selection. (+test) - Terminal focus-suppression + detached-surface guards: suppress autofocus after chrome actions, dismiss the hidden input before chrome so the grid recomputes full-height, and stop a SwiftUI-dismantled surface from doing render/output/a11y work. Ported across GhosttySurfaceView / GhosttySurfaceRepresentable / WorkspaceShellView / WorkspaceDetailContainer / WorkspaceDetailView. Skipped (already on main): nonisolated isolation fixes; .id(terminalID) remount + delegate isolation. Dropped CMUX_PUSH_REDACTED_* (server-side, obsolete). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR enhances sign-out teardown ordering, hardens terminal surface lifecycle management, adds workspace-targeted terminal creation with autofocus suppression, refactors mobile view selection routing, and updates notifications settings with observable state and localization. ChangesSign-out, Terminal Lifecycle, Autofocus, and Mobile Views
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (15 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 |
Contributor
Greptile SummaryThis PR re-implements five targeted iOS fixes from a stale predecessor branch against the current architecture: sign-out push-teardown ordering, push-notification toggle localization and UI state, workspace-scoped terminal creation, and terminal autofocus/keyboard suppression with Ghostty surface lifecycle guards.
Confidence Score: 5/5Safe to merge. All five fixes are well-scoped, tested, and consistent with the current architecture. The auth teardown change is the most sensitive path: it moves the push-unregistration hook before token revocation and bounds it with a structured task group that joins slow teardown before returning, which is both correct and tested with two regression assertions. The terminal autofocus suppression is a store-owned Set with one-shot semantics; the PR adds unit tests for all entry points (create, chrome switch, push deep-link). Localization additions include both supported locales. The Ghostty surface lifecycle changes (dismantle guard, queue-ordered free) are narrow and target a concrete use-after-free risk on fast terminal switches. No dead-state, no timing-based synchronization, and no new actor isolation mistakes were found. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as iOS UI
participant AC as AuthCoordinator
participant Hook as onSignedOut hook (APNs DELETE)
participant Client as StackClient
UI->>AC: signOut()
AC->>+Hook: "addTask { await onSignedOut() }"
AC->>AC: "addTask { sleep(teardownTimeout) }"
Note over AC: withTaskGroup races hook vs timer
alt hook finishes first (normal path)
Hook-->>-AC: completed
AC->>AC: cancelAll() cancels timer
else timer fires first (slow network)
AC->>Hook: cancelAll() sends CancellationError
Hook-->>AC: catches error, exits (joined)
end
AC->>Client: client.signOut() — tokens still valid above
AC->>AC: clearAuthState()
AC-->>UI: sign-out complete
Reviews (9): Last reviewed commit: "Merge origin/main into feat-ios-terminal..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
443
to
453
| public func signOut(onSignedOut: @Sendable () async -> Void = {}) async { | ||
| // Run the teardown hook first, while tokens are still valid (see note). | ||
| await onSignedOut() | ||
| do { | ||
| try await client.signOut() | ||
| } catch { | ||
| authLog.error("Sign-out failed: \(error.localizedDescription, privacy: .private)") | ||
| } | ||
| if launch.includesDevAuth { debugCredentials = nil } | ||
| clearAuthState() | ||
| await onSignedOut() | ||
| } |
Contributor
There was a problem hiding this comment.
Before this change, client.signOut() and clearAuthState() ran unconditionally; the hook was a best-effort cleanup. Now the hook runs first with no timeout at this layer, so if the APNs DELETE hangs (server unreachable, network error, or an indefinite retry in the caller), client.signOut() and clearAuthState() never execute. The user's sign-out request appears to freeze, and the Stack session stays live — exactly the state the PR is trying to prevent. Consider wrapping the hook with a withTimeout or Task.detached so sign-out always completes regardless of hook outcome, while still preserving the token for the DELETE attempt.
Comment on lines
252
to
261
| private func selectTerminalFromPicker(_ terminalID: MobileTerminalPreview.ID) { | ||
| dismissTerminalKeyboardForChrome() | ||
| isTerminalPickerPresented = false | ||
| // Switching to a different terminal is chrome, not a typing intent, so | ||
| // the newly-selected surface must not grab the keyboard on attach. | ||
| if selectedTerminal?.id != terminalID { | ||
| terminalAutoFocusSuppressedSurfaceIDs.insert(terminalID.rawValue) | ||
| } | ||
| suppressNextTerminalAutoFocus = false | ||
| selectedTerminalID = terminalID |
Contributor
There was a problem hiding this comment.
selectTerminalFromPicker always sets suppressNextTerminalAutoFocus = false, but the preceding guard only inserts into terminalAutoFocusSuppressedSurfaceIDs when the selection changes (selectedTerminal?.id != terminalID). If the same terminal is re-confirmed from the picker, the per-surface entry is never added. This means any one-shot suppression set by a concurrent createWorkspaceFromToolbar call (which does not dismiss the picker) is silently cleared with no substitute — the next surface that appears will then autofocus and pop the keyboard. On iPad where the picker popover and the toolbar are both reachable simultaneously, this race is user-triggerable.
![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/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift`:
- Around line 1506-1515: resignInput() currently zeroes keyboardHeight before
keyboardWillHide can run, causing the guard in keyboardWillHide (guard
keyboardHeight != 0) to bail out and skip inputProxy.setKeyboardShown(false) and
toolbar animations; update resignInput() (and the analogous block around
keyboardWillHide handling) to perform the keyboard-hide cleanup before resetting
keyboardHeight: first call inputProxy.setKeyboardShown(false) (or invoke the
same keyboardWillHide cleanup path), then set keyboardHeight = 0 and call
setNeedsGeometrySync(); ensure Self.activeInputSurface handling remains
unchanged and that keyboardWillHide will see a non-zero keyboardHeight when
invoked.
🪄 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: 2f0d3239-f5f7-4eb6-838e-34c6e559b215
📒 Files selected for processing (12)
Packages/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator.swiftPackages/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/AuthCoordinatorTests.swiftPackages/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/Fakes.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellCompositePreviewTests.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileSettingsView.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailContainer.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceShellView.swiftPackages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftios/cmux/Resources/Localizable.xcstrings
Autoreview: the workspace-list '+' (split + compact) created a workspace and mounted its terminal with autoFocusOnWindowAttach still true, popping the keyboard. Route both list-create paths through the same suppression the detail toolbar uses. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…cleanup Autoreview round 2: - AuthCoordinator.signOut: bound the onSignedOut teardown (push-token DELETE) in a task group with a 5s deadline so local sign-out is never gated on a slow/ stuck network call, while still giving the DELETE a chance with valid tokens. - GhosttySurfaceView.resignInput(): stop pre-zeroing keyboardHeight; let resignFirstResponder's keyboardWillHide run the full hide cleanup (toolbar animation, proxy state) instead of short-circuiting it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Autoreview round 3: the round-2 withTaskGroup bound still structurally awaited the teardown child, so a teardown that ignores cancellation (a wedged network DELETE) would keep withTaskGroup from returning and block local sign-out past the deadline. Run the teardown unstructured and only await a deadline it cancels on completion: sign-out proceeds at min(teardown, 5s), and a stuck teardown keeps running detached without holding up the local clear. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rate one Autoreview round 4: disposeSurface() freed the surface on GhosttySurfaceDisposer's own serial queue while process_output / render_now / binding_action run on the shared Self.outputQueue with a captured surface pointer. Two different queues let the free race a queued ghostty_* call on the same pointer (use-after-free during fast terminal switches / removals). Free on Self.outputQueue instead. It's serial and FIFO, so the free is ordered after every already-enqueued block that captured the pointer, and never runs concurrently with one. processOutput's main-actor guard stops new work once surface is nil, so only the bounded backlog drains before the free. Removes the separate-queue disposer (a namespace-enum carve-out) entirely. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Packages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift (2)
1506-1508:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlock geometry scheduling once the surface is dismantled.
After Line 1507, a last
layoutSubviews()/applyViewSize()can still reach Line 1909’s directsyncSurfaceGeometry()path becauseprepareForReuseAfterDetach()has already nilled the display link while the view may still have a window. That schedulesghostty_surface_set_sizework on the sharedoutputQueue, so a SwiftUI-removed surface can still consume the same serial queue that live terminals use forprocess_outputandrender_now. Guard the geometry path onisDismantledand clear any pending geometry state inprepareForDismantle().Suggested fix
public func prepareForDismantle() { isDismantled = true + needsGeometrySync = false + pendingGeometryReassert = false + pendingViewportReport = nil + needsDraw = false prepareForReuseAfterDetach() } private func setNeedsGeometrySync(reassertNaturalSize: Bool = true) { + guard !isDismantled else { return } needsGeometrySync = true if reassertNaturalSize { pendingGeometryReassert = true } needsDraw = true @@ } private func syncSurfaceGeometry(shouldReassertNaturalSize: Bool = true) { - guard let surface else { return } + guard !isDismantled, let surface else { return } @@ let result = GeometryResult(cellPixelSize: cell, naturalSize: natural, pinnedSize: pinnedSize) DispatchQueue.main.async { - self?.applyGeometryResult( - result, - scale: scale, - containerW: containerW, - containerH: containerH, - shouldReassertNaturalSize: shouldReassertNaturalSize - ) + guard let self, !self.isDismantled else { return } + self.applyGeometryResult( + result, + scale: scale, + containerW: containerW, + containerH: containerH, + shouldReassertNaturalSize: shouldReassertNaturalSize + ) } } }🤖 Prompt for 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. In `@Packages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift` around lines 1506 - 1508, Set isDismantled and clear any pending geometry state in prepareForDismantle(): after setting isDismantled = true call code to nil or reset any stored geometry variables and cancel/clear any scheduled geometry work so no ghostty_surface_set_size remains queued on the shared outputQueue; then modify the direct geometry path (the fast path reached from layoutSubviews()/applyViewSize() into syncSurfaceGeometry()) to early-return if isDismantled is true so a dismantled view never schedules ghostty_surface_set_size or touches the outputQueue after prepareForReuseAfterDetach()/prepareForDismantle().
1492-1509: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftStop growing
GhosttySurfaceViewinside this already-oversized file.This PR adds more lifecycle/input-teardown/disposal logic to a production Swift file that already mixes rendering, input, gestures, snapshots, accessibility, registry state, and libghostty lifecycle. Please extract this surface-lifecycle slice into its own type/file instead of extending
GhosttySurfaceViewfurther.As per coding guidelines, "Flag Swift production files that exceed 400 lines without a clear single responsibility, or exceed 800 lines even with mostly coherent responsibility" and "One major type per file."
Also applies to: 1515-1523, 1643-1663
🤖 Prompt for 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. In `@Packages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift` around lines 1492 - 1509, This file is growing and the lifecycle/input-teardown/disposal logic should be moved out of GhosttySurfaceView: create a new type (e.g., GhosttySurfaceLifecycle or GhosttySurfaceController) in its own file and move the lifecycle methods and related state — resignInput(), prepareForDismantle(), prepareForReuseAfterDetach(), isDismandled flag, any references to inputProxy, Self.activeInputSurface, and keyboard-related state/guards — into that new type; update GhosttySurfaceView to hold/forward to the new lifecycle object (delegation/composition) so all callers use the same API but the large lifecycle responsibilities are removed from GhosttySurfaceView. Ensure ownership of activeInputSurface semantics and keyboard hide/cleanup behavior are preserved and referenced symbols remain unchanged so external callers compile.Source: Coding guidelines
🤖 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/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator.swift`:
- Around line 450-459: The sign-out hook Task is inheriting MainActor from the
surrounding `@MainActor` method so a long-running synchronous onSignedOut() can
block MainActor and bypass the 5s deadline; change the child Task that runs the
hook to a non-inheriting task (use Task.detached) so onSignedOut() starts off
the MainActor and deadline.cancel() is still called when it finishes, i.e.
replace the Task { await onSignedOut(); deadline.cancel() } with a Task.detached
{ await onSignedOut(); deadline.cancel() } (referencing the onSignedOut() call
and the deadline Task variable).
---
Outside diff comments:
In
`@Packages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift`:
- Around line 1506-1508: Set isDismantled and clear any pending geometry state
in prepareForDismantle(): after setting isDismantled = true call code to nil or
reset any stored geometry variables and cancel/clear any scheduled geometry work
so no ghostty_surface_set_size remains queued on the shared outputQueue; then
modify the direct geometry path (the fast path reached from
layoutSubviews()/applyViewSize() into syncSurfaceGeometry()) to early-return if
isDismantled is true so a dismantled view never schedules
ghostty_surface_set_size or touches the outputQueue after
prepareForReuseAfterDetach()/prepareForDismantle().
- Around line 1492-1509: This file is growing and the
lifecycle/input-teardown/disposal logic should be moved out of
GhosttySurfaceView: create a new type (e.g., GhosttySurfaceLifecycle or
GhosttySurfaceController) in its own file and move the lifecycle methods and
related state — resignInput(), prepareForDismantle(),
prepareForReuseAfterDetach(), isDismandled flag, any references to inputProxy,
Self.activeInputSurface, and keyboard-related state/guards — into that new type;
update GhosttySurfaceView to hold/forward to the new lifecycle object
(delegation/composition) so all callers use the same API but the large lifecycle
responsibilities are removed from GhosttySurfaceView. Ensure ownership of
activeInputSurface semantics and keyboard hide/cleanup behavior are preserved
and referenced symbols remain unchanged so external callers compile.
🪄 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: dd04db22-6835-4bd1-8608-8fa946484d23
📒 Files selected for processing (3)
Packages/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceShellView.swiftPackages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift
Autoreview round 5: the round-4 unstructured teardown could outlive signOut(). unregisterFromServer() builds its DELETE from the LIVE AuthCoordinator's tokens (PushRegistrationService.makeRequest reads tokenProvider at request-build time), so a delayed stale task that ran after a subsequent sign-in would authenticate the device-token DELETE with the NEW account's credentials and break push for that session. Make the teardown structured: a task group runs the hook + a bounded deadline, then cancelAll() once either finishes; the group joins the (cancelled) hook before returning, so no teardown ever outlives signOut. The push DELETE runs on URLSession (cancellation-aware), so cancelAll() unblocks the join promptly, and awaiting inline guarantees the hook reads this account's tokens since no new sign-in can interleave before signOut returns. (Reverses the round-3 detached approach, which traded round-5's severe cross-account corruption for a hypothetical block by a hook that ignores cancellation; the real hook cancels.) The deadline is an injected Duration (teardownTimeout, default 5s) per the bounded-delay carve-out, so the new signOutJoinsAndCancelsSlowTeardownAtDeadline test exercises the cancel-and-join path in 50ms with no real waiting. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
lawrencecchen
dismissed stale reviews from coderabbitai[bot] and coderabbitai[bot]
June 6, 2026 11:31
Dismissed: CodeRabbit now posts non-blocking comment reviews (request_changes_workflow=false, #5538).
Moves the "don't autofocus a freshly-created terminal" suppression out of WorkspaceShellView's @State (a free-floating `suppressNextTerminalAutoFocus` boolean plus a convert-on-next-selection dance) and into MobileShellComposite, keyed by terminal id. The boolean could not tell a chrome create from a push-notification deep link (both arrive as the same selection change), so any "suppress whatever comes next" scheme sometimes suppressed the wrong surface; it also leaked when a create failed and the selection never changed. This commit lands the store API (`shouldAutoFocusTerminalSurface`, `consumeTerminalAutoFocusSuppression`, `selectTerminalFromChrome`), migrates the three views to read it, and adds behavior tests, but does NOT yet wire the create paths to suppress the new terminal id. The "created terminal is suppressed" tests therefore fail here (red); the next commit adds the wiring. Also reorders createTerminal(in:) so a remote create that is going to abort (another create already in flight) bails before pinning selectedWorkspaceID, so a second "+" can't strand the UI on a workspace with no new terminal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Every create path (local + remote createWorkspace/createTerminal) now marks the freshly-created terminal id in the store the instant it becomes the selection, so its surface mounts with autofocus disabled. This makes the previous commit's tests pass (green) and dissolves the bot-flagged edge cases of the old boolean: - A create that fails (remote RPC error) never produces a terminal id, so there is nothing to leak and the current terminal stays autofocusable. - A push-notification deep link goes through selectTerminal, which is left out of the suppression set, so it autofocuses even mid-create. - Re-confirming the already-selected terminal in the picker is a no-op suppression (selectTerminalFromChrome only suppresses an actual switch). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
lawrencecchen
force-pushed
the
feat-ios-terminal-notify-fixes
branch
from
June 6, 2026 12:13
5da5350 to
ceb5f45
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ceb5f45. Configure here.
Contributor
Author
|
Addressed the remaining open review findings (the AuthCoordinator sign-out and Findings 1-3 (autofocus suppression) were one root cause. The
Finding 4: Suppression is now store state, so it is unit-tested (4 behavior tests in |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (1)
778-783: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd full DocC callouts for the new public APIs.
These package-level public functions have summaries, but the package docs policy also requires
- Parameter/- Returns:callouts on funcs that take parameters or return values. Please fill those in forcreateTerminal(in:),selectTerminalFromChrome(_:),shouldAutoFocusTerminalSurface(_:), andconsumeTerminalAutoFocusSuppression(for:).As per coding guidelines, "Every public symbol in any new Swift package under
Packages/is documented..." and "Use- Parameter name:/- Returns:/- Throws:callouts oninitandfuncsymbols that take parameters or throw."Also applies to: 820-845
🤖 Prompt for 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. In `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 778 - 783, Add DocC callouts for the public functions in MobileShellComposite: update the documentation for createTerminal(in:), selectTerminalFromChrome(_:), shouldAutoFocusTerminalSurface(_:), and consumeTerminalAutoFocusSuppression(for:) to include - Parameter entries for each parameter and - Returns: where the function returns a value, and - Throws: if any throw; ensure the callouts follow the existing summary style and use the exact symbol names (createTerminal(in:), selectTerminalFromChrome(_:), shouldAutoFocusTerminalSurface(_:), consumeTerminalAutoFocusSuppression(for:)) so the package-level public API has complete DocC parameter/return documentation per the package docs policy.Source: Coding guidelines
Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift (1)
44-59:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConsume the suppression token after the surface actually attaches, not in
onAppear.The autofocus gate lives in
GhosttySurfaceView.didMoveToWindow(), but this clears the token from SwiftUIonAppear. That makes the feature timing-dependent: ifonAppearfires, triggers a rerender, andupdateUIViewflipsautoFocusOnWindowAttachback totruebefore the UIView reachesdidMoveToWindow(), the keyboard still pops on the very mount this PR is trying to suppress. Move the consume step into the hosted-view/coordinator attach path that runs after the surface is in a window.🤖 Prompt for 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. In `@Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift` around lines 44 - 59, The current call to store.consumeTerminalAutoFocusSuppression(for: terminalID) in WorkspaceDetailView's onAppear is too early; move the consume step into the GhosttySurfaceRepresentable's hosted view attach path so it runs after the UIView is in a window. Specifically, remove the consumeTerminalAutoFocusSuppression call from the .onAppear block in WorkspaceDetailView and instead invoke store.consumeTerminalAutoFocusSuppression(for:) from the GhosttySurfaceView/Coordinator attachment flow (e.g., in GhosttySurfaceView.didMoveToWindow() or the coordinator's view attach handler that runs after makeUIView/dismantleUIView) so the suppression token is cleared only after the surface is attached and the didMoveToWindow-based autofocus gate can operate correctly.
🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 90-100: terminalAutoFocusSuppressedSurfaceIDs persists across
session resets and can suppress autofocus in a new connection; update signOut()
and clearRemoteConnectionContext() to clear
terminalAutoFocusSuppressedSurfaceIDs when rebuilding session state OR modify
logic that uses terminalAutoFocusSuppressedSurfaceIDs to include the current
connectionGeneration so suppressed IDs are only valid for that generation (e.g.,
pair the set with connectionGeneration or check connectionGeneration when
consuming/consulting the set from consumeTerminalAutoFocusSuppression(for:)).
Ensure you reference and clear/update terminalAutoFocusSuppressedSurfaceIDs
inside the same reset code paths (signOut() and clearRemoteConnectionContext())
so stale IDs cannot carry over.
---
Outside diff comments:
In `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 778-783: Add DocC callouts for the public functions in
MobileShellComposite: update the documentation for createTerminal(in:),
selectTerminalFromChrome(_:), shouldAutoFocusTerminalSurface(_:), and
consumeTerminalAutoFocusSuppression(for:) to include - Parameter entries for
each parameter and - Returns: where the function returns a value, and - Throws:
if any throw; ensure the callouts follow the existing summary style and use the
exact symbol names (createTerminal(in:), selectTerminalFromChrome(_:),
shouldAutoFocusTerminalSurface(_:), consumeTerminalAutoFocusSuppression(for:))
so the package-level public API has complete DocC parameter/return documentation
per the package docs policy.
In
`@Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift`:
- Around line 44-59: The current call to
store.consumeTerminalAutoFocusSuppression(for: terminalID) in
WorkspaceDetailView's onAppear is too early; move the consume step into the
GhosttySurfaceRepresentable's hosted view attach path so it runs after the
UIView is in a window. Specifically, remove the
consumeTerminalAutoFocusSuppression call from the .onAppear block in
WorkspaceDetailView and instead invoke
store.consumeTerminalAutoFocusSuppression(for:) from the
GhosttySurfaceView/Coordinator attachment flow (e.g., in
GhosttySurfaceView.didMoveToWindow() or the coordinator's view attach handler
that runs after makeUIView/dismantleUIView) so the suppression token is cleared
only after the surface is attached and the didMoveToWindow-based autofocus gate
can operate correctly.
🪄 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: e3471501-50c9-4815-a781-6b1bbe48645b
📒 Files selected for processing (8)
Packages/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator.swiftPackages/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/AuthCoordinatorTests.swiftPackages/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/Fakes.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellCompositePreviewTests.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailContainer.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceShellView.swift
Comment on lines
+90
to
+100
| /// Surface IDs whose next window attach must NOT grab the keyboard. | ||
| /// | ||
| /// A surface in this set mounts with autofocus disabled; the entry is | ||
| /// cleared once that surface has appeared and consumed the suppression | ||
| /// (``consumeTerminalAutoFocusSuppression(for:)``). Ownership lives here, | ||
| /// next to selection and terminal creation, rather than in the view, so the | ||
| /// create path can mark the *exact* new terminal id the instant it becomes | ||
| /// the selection. A freshly created terminal therefore never steals the | ||
| /// keyboard, while push-notification navigation (``selectTerminal(_:)``) is | ||
| /// intentionally left out of the set and allowed to autofocus. | ||
| private var terminalAutoFocusSuppressedSurfaceIDs: Set<String> = [] |
There was a problem hiding this comment.
Clear pending autofocus suppression when shell state is reset.
terminalAutoFocusSuppressedSurfaceIDs is now long-lived state, but signOut() and clearRemoteConnectionContext() below rebuild the session without emptying it. If a chrome-driven selection is queued and the surface never reaches consumeTerminalAutoFocusSuppression(for:) before teardown, that stale surface ID survives into the next connection and suppresses autofocus for a later mount too. Reset this set alongside the rest of the selection/connection state, or scope it to the current connectionGeneration.
🤖 Prompt for 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.
In `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`
around lines 90 - 100, terminalAutoFocusSuppressedSurfaceIDs persists across
session resets and can suppress autofocus in a new connection; update signOut()
and clearRemoteConnectionContext() to clear
terminalAutoFocusSuppressedSurfaceIDs when rebuilding session state OR modify
logic that uses terminalAutoFocusSuppressedSurfaceIDs to include the current
connectionGeneration so suppressed IDs are only valid for that generation (e.g.,
pair the set with connectionGeneration or check connectionGeneration when
consuming/consulting the set from consumeTerminalAutoFocusSuppression(for:)).
Ensure you reference and clear/update terminalAutoFocusSuppressedSurfaceIDs
inside the same reset code paths (signOut() and clearRemoteConnectionContext())
so stale IDs cannot carry over.
main evolved the iOS shell heavily (multi-Mac host switcher, terminal toolbar redesign, rename/pin workspaces); none of it had the autofocus-suppression feature this branch introduces. Resolved: - WorkspaceShellView.swift: took main's version (rename/pin closures, toolbar); the branch's only net change here was an inert createWorkspaceFromSplitList wrapper, now unneeded since suppression lives in the store. - MobileSettingsView.swift: kept both new @State (branch's notificationsEnabled toggle mirror + main's showingHostPicker). WorkspaceDetailView/Container, GhosttySurfaceRepresentable, and the store auto-merged: the store-based suppression consumption sits alongside main's toolbar changes with no overlap. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jun 7, 2026
This was referenced Jun 10, 2026
Merged
This was referenced Jun 18, 2026
This was referenced Jun 26, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Re-implements the still-relevant fixes from #5259, which was closed because it was 569 commits behind main and edited files the iOS refactor deleted (
ios/cmuxPackage/.../WorkspaceViews.swift,CmuxMobileAuth/AuthManager.swift) — a rebase would have been a bug-prone rewrite. I audited all 16 commits against current main and re-did the 5 still-needed fixes against the new architecture.Fixes
AuthCoordinator.signOutnow runs theonSignedOuthook (the APNs device-token DELETE) before revoking the Stack session, so the DELETE still has a valid token. Previously it ran after revocation and was silently skipped, leaving the device receiving pushes for a signed-out account. (+unit test asserting the hook sees a valid token.)mobile.notifications.enable/mobile.notifications.disable(en + ja) toios/cmux/Resources/Localizable.xcstrings(the call site already referenced them; they were falling back to English with no JA).MobileSettingsViewmirrorsMobilePushCoordinator.isEnabledinto@Stateso the label/icon refresh after the async enable/disable (isEnabledis a non-observableUserDefaultsread).createTerminal(in:). Targets an explicit workspace so an in-flight create can't land in a drifted selection. (+unit test.)GhosttySurfaceView,GhosttySurfaceRepresentable,WorkspaceShellView,WorkspaceDetailContainer,WorkspaceDetailView.Skipped (already on main / obsolete)
nonisolatedisolation fixes and.id(terminalID)remount + notification-delegate isolation are already on main.CMUX_PUSH_REDACTED_*dropped — the redacted push body is generated server-side (web/services/apns).🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches auth sign-out ordering (push teardown), libghostty surface free queue ordering, and terminal input/focus paths—important for correctness and native crashes, but scoped with regression tests and bounded teardown.
Overview
Ports still-needed iOS fixes onto the refactored shell/auth stack: sign-out, mobile terminal UX, notifications settings, and Ghostty surface lifecycle.
Sign-out:
AuthCoordinator.signOutnow runs the composition-rootonSignedOuthook (e.g. authenticated APNs device-token DELETE) beforeclient.signOut()revokes tokens, with a 5s bounded task group that joins and cancels slow teardown so sign-out cannot hang and teardown cannot outlive the session. Tests cover token visibility during the hook and deadline cancellation.Mobile shell / terminal:
MobileShellCompositeadds one-shot autofocus suppression for chrome paths (create workspace/terminal, terminal picker viaselectTerminalFromChrome); push deep links still useselectTerminaland may autofocus.createTerminal(in:)pins the target workspace for async remote creates. UI wires suppression throughGhosttySurfaceRepresentable, resigns input before chrome (GhosttySurfaceView.resignActiveInput()), and passes explicit workspace id fromWorkspaceDetailContainer.Ghostty: Surfaces honor
autoFocusOnWindowAttach,prepareForDismantlestops render/output/a11y after SwiftUI removal, and libghosttyghostty_surface_freeruns on the sharedoutputQueue(replacing a separate disposer queue) to avoid use-after-free with queued render/output work.Settings / l10n:
MobileSettingsViewmirrors push enable state in@State(seeded on appear) so the toggle label updates after async enable/disable; adds en/ja strings for notification enable/disable labels.Reviewed by Cursor Bugbot for commit 3b7fc5b. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Re-implements iOS fixes for secure sign-out, scoped terminal creates, predictable keyboard behavior, and safer terminal teardown; merged with main’s new host picker/toolbar. Adds localized notification labels and keeps the settings toggle in sync.
mobile.notifications.enable/mobile.notifications.disable(en + ja); mirror push state into@State(seeded on appear) so the label/icon update after enable/disable.createTerminal(in:)pins creates to a specific workspace; store tracks one‑shot autofocus suppression per terminal id so creates and picker switches don’t pop the keyboard while push deep‑links still do; suppression is passed viaautoFocusOnWindowAttachand consumed on appear;GhosttySurfaceView.resignActiveInput()is called before overlays withkeyboardWillHideowning cleanup.process_output/render_now) to avoid use‑after‑free; removed the separate disposer queue; integrates cleanly with main’s host switcher and toolbar.Written for commit 3b7fc5b. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
New Features
Improvements