Repository navigation
iOS: keep the bottom bars docked to the keyboard when the layout guide misses a transition - #9663
azooz2003-bit wants to merge 14 commits into
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 process-wide UIKit keyboard frame tracker and uses its overlap data to maintain ChangesKeyboard dock floor
Injected attach startup admission
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant KeyboardNotifications
participant MobileKeyboardFrameTracker
participant GhosttySurfaceView
participant ComposerConstraints
KeyboardNotifications->>MobileKeyboardFrameTracker: record keyboard frame transition
KeyboardNotifications->>GhosttySurfaceView: handle keyboard frame change
GhosttySurfaceView->>MobileKeyboardFrameTracker: read overlap
GhosttySurfaceView->>ComposerConstraints: apply overlap floor
ComposerConstraints->>GhosttySurfaceView: reserve maximum guide or floor overlap
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors)
✅ Passed checks (22 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 |
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/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift`:
- Around line 525-529: Remove production test seams: make
GhosttySurfaceView.keyboardFrameTracker a private let injected through its
normal initializer with .shared as the default, and make
handleKeyboardWillChangeFrame internal while deleting
handleKeyboardWillChangeFrameForTesting. In
Packages/iOS/CmuxMobileTerminal/Tests/CmuxMobileTerminalTests/GhosttySurfaceKeyboardDockFloorTests.swift
lines 95-97 and 159-161, call the internal handler directly; at lines 112-116,
pass the isolated tracker through the initializer.
🪄 Autofix
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 Plus
Run ID: 8e4a00c4-e250-4da2-80e4-9fcc7ac297c9
📒 Files selected for processing (4)
Packages/iOS/CmuxMobileSupport/Sources/CmuxMobileSupport/MobileKeyboardFrameTracker.swiftPackages/iOS/CmuxMobileSupport/Tests/CmuxMobileSupportTests/MobileKeyboardFrameTrackerTests.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftPackages/iOS/CmuxMobileTerminal/Tests/CmuxMobileTerminalTests/GhosttySurfaceKeyboardDockFloorTests.swift
f41d974 to
2298d2a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
307427e to
1863c2c
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/CmuxMobileSupport/Sources/CmuxMobileSupport/MobileKeyboardFrameTracker.swift`:
- Around line 17-25: Remove the process-wide MobileKeyboardFrameTracker.shared
singleton and its singleton-specific documentation. Instantiate the tracker from
the app or scene lifecycle owner, then pass the immutable tracker reference into
each GhosttySurfaceView through its existing injection path, preserving shared
observation without ambient global state.
- Around line 47-52: The MobileKeyboardFrameTracker must clear stale keyboard
overlap when a notification lacks a usable end frame. In
MobileKeyboardFrameTracker.swift lines 47-52, set latestTransition to nil before
returning from the failed transition parse; in
MobileKeyboardFrameTrackerTests.swift lines 112-125, update the regression test
to expect a cleared transition.
🪄 Autofix
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 Plus
Run ID: 2e3bfd50-646c-45d2-9cb5-21d0563dcc9f
📒 Files selected for processing (5)
Packages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/MobileInjectedAttachStartupTests.swiftPackages/iOS/CmuxMobileSupport/Sources/CmuxMobileSupport/MobileKeyboardFrameTracker.swiftPackages/iOS/CmuxMobileSupport/Tests/CmuxMobileSupportTests/MobileKeyboardFrameTrackerTests.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftPackages/iOS/CmuxMobileTerminal/Tests/CmuxMobileTerminalTests/GhosttySurfaceKeyboardDockFloorTests.swift
Field repro (IMG_6733.mov): open a workspace with the keyboard recently up, focus input, the keyboard rises but the accessory toolbar and composer band stay seated at the screen bottom behind it. The dock is constrained to UIView.keyboardLayoutGuide, which only reflects transitions UIKit routed to the view's window while it was installed; a test window's guide never moves, which reproduces the missed-transition wedge deterministically. Adds MobileKeyboardFrameTracker (inert in this commit), a DEBUG-only tracker-injection seam, and a per-view notification test seam. The dock tests are red until the dock stops depending on the guide alone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The dock (accessory toolbar + composer band) follows UIView.keyboardLayoutGuide. The guide only observes keyboard transitions UIKit routed to the view's window while the view was installed, so a surface (re)attached around a workspace switch can miss the rise entirely and stay seated on the guide's bottom-safe-area fallback while the keyboard covers it — the bars vanish behind the keyboard and the grid keeps keyboard-down geometry (IMG_6733.mov). Keyboard notifications are posted process-wide regardless of attachment, so MobileKeyboardFrameTracker records the latest keyboard end frame as the floor's single data source, and the dock gains a REQUIRED inequality floor (composer.bottom <= view.bottom - tracked overlap) beneath the guide equality, which drops one priority notch. The guide remains the movement engine; the floor only stops the dock from sitting below the real keyboard. The per-view notification handler re-reads the tracker on the notification's own animation curve, and layout passes catch late-attached views up from the same tracker, so the two paths can never disagree. The viewport model takes max(guide, floor) so the terminal grid reserves exactly the space the lifted bars occupy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The suite was written against a connectInjectedAttach(_:attachURL:_:) coordinator API that was reworked before #9252 merged, so the file has never compiled. cmux.xctestplan builds every member test target even under -only-testing, so this one broken target has been failing the whole 'iOS simulator tests' iphone lane for every dispatch since. Rewrite the suite against the coordinator API that shipped, preserving the admission contract: a connected injected attach consumes startup and blocks the saved-Mac reconnect, a failed one releases startup to it, and only one startup source can claim admission. The original's URL-connect side-effect assertion lives in CMUXMobileRootView's path and was never compilable here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1863c2c to
b74ab85
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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/Tests/CmuxMobileShellUITests/MobileInjectedAttachStartupTests.swift`:
- Around line 14-29: Add a focused test alongside
connectedInjectedAttachConsumesStartupWithoutFallback that claims an injected
attach and finishes it with outcome .awaitingUserApproval. Assert it returns
false, leaves shouldFallBackFromInjectedAttach false, and makes
claimStoredReconnect() return nil, matching the behavior in
MobileStartupConnectionCoordinator.finishInjectedAttach.
🪄 Autofix
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 Plus
Run ID: 528cf274-b540-44df-ad20-e70c8c55d6b2
📒 Files selected for processing (5)
Packages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/MobileInjectedAttachStartupTests.swiftPackages/iOS/CmuxMobileSupport/Sources/CmuxMobileSupport/MobileKeyboardFrameTracker.swiftPackages/iOS/CmuxMobileSupport/Tests/CmuxMobileSupportTests/MobileKeyboardFrameTrackerTests.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftPackages/iOS/CmuxMobileTerminal/Tests/CmuxMobileTerminalTests/GhosttySurfaceKeyboardDockFloorTests.swift
Review feedback (CodeRabbit): drop the DEBUG-only tracker swap and the ForTesting notification wrapper. The surface now takes the tracker as an immutable init dependency defaulting to the shared process-wide instance, tests inject a notification-center-isolated tracker through the initializer and call the internal handler via @testable import. A keyboardWillChangeFrame without a readable end frame now clears the tracked transition (fail closed) instead of preserving a stale floor. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review feedback (CodeRabbit): finishInjectedAttach treats .awaitingUserApproval like .connected — startup stays consumed and the saved-Mac reconnect must not dial under the approval prompt. Only .connected was covered, so a regression in the approval arm could pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Leaving the workspace detail with the keyboard up and re-entering shows the hide-keyboard glyph (and keyboardUp=1) although the keyboard is down: the toolbar button is constructed in the keyboard-up state and nothing reconciles the visibility bit when a surface (re)mounts without a keyboard event. Red until the visibility state catches up from the tracked keyboard frame. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The toolbar's keyboard-toggle glyph and the surface's keyboardVisible bit were only updated from keyboard notifications, so a surface (re)mounted after the keyboard changed kept the stale state: re-entering a workspace left with the keyboard up showed the hide-keyboard glyph over a dismissed keyboard, and the toggle resigned a keyboard that was not there. The dismiss button is now born in the keyboard-down state, and the layout catch-up that already re-derives the dock floor from MobileKeyboardFrameTracker also reconciles keyboardVisible (change-guarded so the glyph cross-dissolve only runs on real flips). The tracker gains isVisible(in:), mirroring the notification path's floating-keyboard semantics. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Second fix on this branch (same state-ownership family, reported during dogfood): leaving the workspace detail with the keyboard up and re-entering showed the stale hide-keyboard glyph over a dismissed keyboard, and the toggle resigned a keyboard that was not there. Cause: the toolbar's keyboard button is constructed in the keyboard-up state and |
Dogfood recording (2026-08-05 20:48): the keyboard rose over the bars, which snapped up only after it settled — on every rise, including rapid toggles. With usesBottomSafeArea=true the keyboard guide's position is coupled to bottom-safe-area propagation, which lands at the END of a keyboard transition, so the guide-constrained dock moved late; the same coupling explains the settled guide reading the bottom inset below the notification frame on the simulator. Switch the guide to pure keyboard tracking (usesBottomSafeArea=false) and express the keyboard-down seat explicitly: a required cap keeps the dock above the bottom safe area, decisive only while the keyboard is down (the guide equality and notification floor are stricter when it is up). The existing dismissal test covers the cap (it fails without it in the new guide mode). DEBUG forensics for this class of bug: kb.willChange logs the notification frame, tracker/guide/floor trio, and duration; kb.floor marks late floor application, so a recording can be lined up against which source moved the dock and when. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Device forensics (kb.willChange/kb.floor, 2026-08-06): every keyboard rise applied the dock floor at willChangeFrame with an overlap 34pt short of the settled truth, corrected ~1s later by a layout catch-up — the bars visibly popped by the home-indicator height after each rise. Cause: the surface ignored only the KEYBOARD safe area, so its bottom edge respected the home indicator while the keyboard was down but extended to the window bottom while it was up (the keyboard region subsumes the indicator inset). The frame breathed by 34pt on every transition, and converting the (final) notification end frame through the mid-animation frame under-measured. Extend the surface under the home indicator in ALL states so its frame is keyboard-invariant; the dock's required safe-area cap (previous commit) keeps the bars clear of the indicator while the keyboard is down, and the grid already reserves the bottom safe area. The tracker also observes keyboardDidChangeFrameNotification so any consumer converting through a view that DID move mid-transition converges on settled geometry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
When the phone reappears after being away, the queue drained every pending tag with a signed foreground launch, each suspending the previously launched app seconds after it paired — its stream dropped and the user found their dogfood app disconnected (2026-08-06 13:41-13:43: four tags launched back to back). Launch only the newest queued build per drain pass; older ones install fresh but stay unlaunched and re-pair with stored credentials on their next manual open. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tles Device forensics: the host resizes the surface by the home-indicator height across every keyboard transition (bounds flipping 836<->802), so a floor seeded from a mid-transition view-space conversion lands 34pt shy and pops into place a second later. Red until the floor is window-anchored. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The surface's frame is not keyboard-invariant and cannot be made so at the leaf: safe-area propagation resizes it by the home-indicator height across every keyboard transition (device bounds flipping 836<->802 even with the container-bottom ignore), so a floor expressed against the surface's bottom was seeded from a mid-transition conversion 34pt shy of the settled truth and popped into place when a later layout pass re-derived it. Anchor the floor constraint to the WINDOW — the only frame that never moves — installed per window attach and dropped on detach. Its constant is the notification's window-space overlap (MobileKeyboardTransition. overlapInWindow(of:)), final on first application, so the bars ride the keyboard's animation curve with no late correction. The viewport model converts the window floor into view space per layout pass, converging with the surface's settled frame. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Rounds of custom dock geometry all failed the same way: some input lied mid-transition (keyboardLayoutGuide frozen or late on device, the surface's frame breathing by the home-indicator height, notification frames converted through that moving frame). Change the owner instead of the inputs: the toolbar row + composer band now live in one self-sizing UIInputView (KeyboardDockAccessoryView) that every cmux keyboard owner returns as its inputAccessoryView — the terminal input proxy while typing, and the surface itself (canBecomeFirstResponder) in the keyboard-down state, so the system docks the bar at the screen bottom and carries it on the keyboard's own animation in every transition, interactive gestures included. Deleted for the bars: the guide equality and configuration, the notification floor constraint, the safe-area cap, and all dock position math. The tracker now feeds only the terminal grid reservation, with the accessory's own footprint subtracted (keyboard end frames include the accessory, which would otherwise double-reserve the dock and make the docked-accessory state read as a visible keyboard). Dock tests rewritten to the accessory contract: responder wiring, chrome withholding, docked-state first responder, model derivation, and the glyph reconcile coverage carried over. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 670b0dd. Configure here.
| let keyboardUp = try #require(probeValue(of: harness.view, key: "keyboardUp")) | ||
| #expect(abs(modelKeyboardHeight - Self.keyboardHeight) <= 1) | ||
| // The visibility bit catches up too: the toggle must read hide-keyboard. | ||
| #expect(keyboardUp == 1) |
There was a problem hiding this comment.
Tests assert full keyboard overlap
Medium Severity
The dock regression tests expect keyboardHeight to equal the full tracked window overlap, but production derives that model by subtracting keyboardDockAccessory.contentHeight so the grid can reserve the toolbar and composer separately. With a non-zero accessory footprint those assertions cannot pass, so the rise and late-attach coverage does not validate the real contract.
Reviewed by Cursor Bugbot for commit 670b0dd. Configure here.
| ) | ||
| let viewMaxYInWindow = convert(bounds, to: window).maxY | ||
| let belowView = max(0, window.bounds.maxY - viewMaxYInWindow) | ||
| return max(0, keyboardOnly - belowView) |
There was a problem hiding this comment.
Stale floor under-reserves composer growth
Medium Severity
keyboardHeight is computed as tracked overlap minus the live accessory contentHeight, while setComposerBandHeight updates that content height without refreshing the tracked frame. If the accessory grows and UIKit does not immediately post a new keyboard frame, total grid reservation stays at the old overlap even though the OS-hosted dock has moved farther up over the terminal.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 670b0dd. Configure here.
Rapid successive toggles (Aziz's overnight repro) desynced: keyboardVisible is reconciled from keyboard notifications and lags during back-to-back transitions, so a quick re-tap re-focused when it should resign until the keyboard wedged against the button. Branch on the actual first responder instead; the tracked bit remains display-only (glyph). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>


Bug
Keyboard rises but the bottom bars (Tab/Esc accessory toolbar + Message composer) stay seated at the screen bottom behind it. Field recording repro: keyboard up in workspace A, back to the list, open workspace B, focus input — the keyboard covers the bars and the terminal grid keeps keyboard-down geometry until the keyboard is dismissed and re-raised.
Root cause
Since #9371 the dock is constrained to
UIView.keyboardLayoutGuide, which only reflects keyboard transitions UIKit routed to the view's window while the view was installed. A surface (re)attached around a workspace switch can miss the transition and stay seated on the guide's bottom-safe-area fallback while the keyboard is up. Nothing re-derives keyboard geometry after attach, so both the constraint-positioned bars and thekeyboardOverlapFromLayoutGuideviewport model stay at keyboard-down layout.Fix
Keyboard notifications are posted process-wide regardless of any view's attachment, so:
MobileKeyboardFrameTracker(CmuxMobileSupport) records the latest keyboard end frame fromkeyboardWillChangeFrame, clearing onkeyboardDidHideand backgrounding.composer.bottom <= view.bottom - trackedOverlapbeneath the guide equality, which drops one priority notch. The guide remains the movement engine; the floor only stops the dock from sitting below the real keyboard. It animates on the notification's own curve on the notification path and catches up from the tracker on layout after a late attach.max(guide, floor)so the terminal grid reserves exactly the space the lifted bars occupy.A stale notification frame can never pin the dock below wherever the guide places it (inequality, not a second equality authority), and
MobileKeyboardReservationkeeps floating/split iPad keyboards at zero overlap.Tests
Commit 1 (red): dock regression tests in a test window, where the system guide never observes a keyboard — deterministically the same wedge as the missed live transition. Covers the notification path, the attach-after-transition path, and dismissal release. Plus
MobileKeyboardFrameTrackerunit tests (notification-center-isolated).Commit 2 (green): the fix.
Localization audit: no user-facing strings added or changed.
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Reworked the iOS bottom dock so the toolbar and composer are a system keyboard accessory that rides the keyboard’s animation and stays docked at the bottom when the keyboard is down. This removes guide/floor glitches, stabilizes transitions, and keeps the grid reservation accurate from a tracker-derived model.
KeyboardDockAccessoryViewreturned as theinputAccessoryViewof both the terminal input proxy and the surface; the OS now positions the dock in all states.MobileKeyboardFrameTrackernow feeds only the terminal grid model using window-space overlap (with the accessory’s height subtracted), clears on hide/background, and reconciles visibility on remount so the toggle starts as “Show Keyboard.”.ignoresSafeArea(.container, edges: .bottom)) to keep frame conversions stable mid-animation.Written for commit 3fab1b1. Summary will update on new commits.
Summary by CodeRabbit
Note
Medium Risk
Large change to terminal input, first-responder, and keyboard geometry on a user-critical path; mitigated by extensive new tests and unchanged grid math semantics aside from the positioning model shift.
Overview
Fixes iOS terminal bottom chrome (toolbar + composer) lagging or sitting behind the keyboard after workspace switches by moving dock positioning to the OS keyboard system instead of
keyboardLayoutGuideconstraints on the surface.The toolbar and composer band now live in a self-sizing
KeyboardDockAccessoryViewreturned asinputAccessoryViewfrom the surface and the hidden typing proxy, with the surface holding first responder when the keyboard is down so the dock stays at the screen bottom. Grid reservation still follows keyboard height via a new process-wideMobileKeyboardFrameTracker(notification-based, catch-up on late attach) and subtracts the accessory footprint so the dock does not double-count.WorkspaceDetailViewadds.ignoresSafeArea(.container, edges: .bottom)so the terminal surface frame does not breathe with the home indicator mid-transition (which skewed keyboard conversions and caused visible bar pops).Tests:
MobileInjectedAttachStartupTestsis rewritten for the shipped startup coordinator API (restores a compile-broken target). New tracker and keyboard-dock contract tests cover accessory wiring, overlap model, and reattach visibility.Dev tooling:
iphone-install-queue.shinstalls all pending builds but launches only the newest per drain to avoid foreground fights on a single device.Reviewed by Cursor Bugbot for commit 670b0dd. Bugbot is set up for automated code reviews on this repo. Configure here.