Fix iOS chat keyboard scroll edge bleed - #7070
Closed
azooz2003-bit wants to merge 44 commits into
Closed
azooz2003-bit wants to merge 44 commits into
azooz2003-bit wants to merge 44 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR adds a configurable accessory shortcut row to the iOS chat composer ( ChangesCmuxMobileSupport: New Public Keyboard/Composer Primitives
Chat Composer: Accessory Shortcut Row
iOS Chat Keyboard Tracking ViewController
Terminal Surface: Keyboard Height Animation Refactor
MobileShellUI: Chat Pane Shortcuts and Demo Screen
UI Tests, Swift Concurrency Fixes, and Script Update
Sequence Diagram(s)sequenceDiagram
participant UIKit as UIKit Keyboard Notification
participant VC as ChatKeyboardTrackingViewController
participant TranscriptTable as ChatTranscriptUITableView
participant Composer as Composer UIHostingController
UIKit->>VC: keyboardWillChangeFrame
VC->>VC: compute effective duration (in-flight vs. remaining distance)
VC->>Composer: UIView.animate → translateY by keyboard overlap
VC->>TranscriptTable: applyComposerOverlayBottomInset(_:)
TranscriptTable->>TranscriptTable: snapshot viewport → update contentInset.bottom → restoreKeyboardViewport
VC->>VC: updateEdgeEffects(overlap:)
VC->>VC: completeKeyboardTransition → pin final geometry
sequenceDiagram
participant Host as WorkspaceChatPane
participant ChatScreen as ChatScreen
participant ChipRow as ChatAccessoryChipRow
participant TerminalSurface as GhosttySurfaceView
Host->>Host: chatAccessoryShortcuts(for: conversation)
Host->>ChatScreen: accessoryShortcuts: [ChatAccessoryShortcut]
ChatScreen->>ChipRow: shortcuts: + leadingShortcuts:
ChipRow-->>Host: shortcut.perform() → performChatAccessoryAction
Host->>TerminalSurface: sendSessionTerminalInput (UTF-8 encoded)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (21 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 replaces the SwiftUI preference-key–based keyboard-dismiss region with a UIKit
Confidence Score: 4/5Safe to merge; the keyboard-tracking architecture is sound and well-tested across three scroll-position variants. The two findings are both style-level: one is a direct ProcessInfo env-var read in a debug helper that bypasses the injectable UITestEnvironmentConfig introduced in the same PR, and the other is a large #if DEBUG block in ChatTranscriptUITableView.swift (accessibility override + debug stored properties) that follows a different isolation pattern than the dedicated +Debug file used for ChatKeyboardTrackingViewController. Neither affects production behavior or release builds. ChatTranscriptUITableView.swift (mixed production/debug code) and the #if DEBUG applyDebugInitialScrollIfNeeded block in ChatTranscriptTableView.swift (direct ProcessInfo access). Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant KB as UIKit Keyboard
participant VC as ChatKeyboardTrackingViewController
participant Anim as UIView.animate
participant Clip as transcriptClipView
participant Table as ChatTranscriptUITableView
participant Edge as UIScrollEdgeEffect
KB->>VC: keyboardWillChangeFrameNotification
VC->>VC: compute overlap(in: view)
VC->>Clip: pinAnimationToVisibleOverlap(startOverlap)
VC->>VC: "isKeyboardAnimationActive = true"
VC->>Anim: "transition.animate { applyKeyboardOverlap(target) }"
Anim->>Clip: CGAffineTransform(translationX:0, y:-overlap)
Anim->>Table: applyComposerOverlayBottomInset(composerHeight)
Table->>Table: restoreKeyboardViewport(snapshot)
Anim->>Edge: "topEdgeEffect.style = .automatic (suppressed)"
Anim->>Edge: "bottomEdgeEffect.style = .soft"
Anim-->>VC: "completion { finishKeyboardAnimation() }"
VC->>VC: "isKeyboardAnimationActive = false"
VC->>Edge: "topEdgeEffect.style = .soft (restored if overlap == 0)"
%%{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 KB as UIKit Keyboard
participant VC as ChatKeyboardTrackingViewController
participant Anim as UIView.animate
participant Clip as transcriptClipView
participant Table as ChatTranscriptUITableView
participant Edge as UIScrollEdgeEffect
KB->>VC: keyboardWillChangeFrameNotification
VC->>VC: compute overlap(in: view)
VC->>Clip: pinAnimationToVisibleOverlap(startOverlap)
VC->>VC: "isKeyboardAnimationActive = true"
VC->>Anim: "transition.animate { applyKeyboardOverlap(target) }"
Anim->>Clip: CGAffineTransform(translationX:0, y:-overlap)
Anim->>Table: applyComposerOverlayBottomInset(composerHeight)
Table->>Table: restoreKeyboardViewport(snapshot)
Anim->>Edge: "topEdgeEffect.style = .automatic (suppressed)"
Anim->>Edge: "bottomEdgeEffect.style = .soft"
Anim-->>VC: "completion { finishKeyboardAnimation() }"
VC->>VC: "isKeyboardAnimationActive = false"
VC->>Edge: "topEdgeEffect.style = .soft (restored if overlap == 0)"
Reviews (1): Last reviewed commit: "Disable chat top scroll edge while keybo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
284
to
+296
| min(max(offsetY, -tableView.adjustedContentInset.top), maxOffsetY(in: tableView)) | ||
| } | ||
|
|
||
| @objc private func keyboardWillChangeFrame(_ notification: Notification) { | ||
| guard let tableView else { return } | ||
| keyboardWasAtBottom = isAtBottom.wrappedValue | ||
| || distanceFromBottom(in: tableView) <= Self.atBottomThreshold | ||
| keyboardVisibleBottomY = visibleBottomY(in: tableView) | ||
| keyboardBottomAnchor = bottomVisibleAnchor(in: tableView) | ||
| keyboardAnimationDuration = Self.keyboardAnimationDuration(from: notification) | ||
| keyboardAnimationOptions = Self.keyboardAnimationOptions(from: notification) | ||
| shouldPreserveKeyboardViewport = true | ||
| } | ||
|
|
||
| @objc private func keyboardDidChangeFrame(_ notification: Notification) { | ||
| guard let tableView, shouldPreserveKeyboardViewport else { return } | ||
| tableView.layoutIfNeeded() | ||
| preserveViewportAfterLayout(in: tableView) | ||
| shouldPreserveKeyboardViewport = false | ||
| keyboardWasAtBottom = false | ||
| keyboardVisibleBottomY = nil | ||
| keyboardBottomAnchor = nil | ||
| keyboardAnimationDuration = 0 | ||
| keyboardAnimationOptions = [] | ||
| } | ||
|
|
||
| private static let atBottomThreshold: CGFloat = 40 | ||
|
|
||
| private static func keyboardAnimationDuration(from notification: Notification) -> TimeInterval { | ||
| notification.userInfo?[UIResponder.keyboardAnimationDurationUserInfoKey] as? TimeInterval ?? 0 | ||
| #if DEBUG | ||
| private func applyDebugInitialScrollIfNeeded(in tableView: UITableView) { | ||
| guard !didApplyDebugInitialScroll, | ||
| ProcessInfo.processInfo.environment["CMUX_UITEST_CHAT_INITIAL_SCROLL"] == "middle", | ||
| tableView.bounds.height > 0, | ||
| tableView.contentSize.height > tableView.bounds.height * 1.4 | ||
| else { | ||
| return | ||
| } | ||
| didApplyDebugInitialScroll = true |
Contributor
There was a problem hiding this comment.
applyDebugInitialScrollIfNeeded bypasses UITestEnvironmentConfig
This function reads ProcessInfo.processInfo.environment["CMUX_UITEST_CHAT_INITIAL_SCROLL"] directly while the same PR introduces UITestEnvironmentConfig specifically as an injectable abstraction for these env vars. The raw ProcessInfo access makes the guard untestable in isolation and creates an inconsistency: all other new env-var checks go through UITestEnvironmentConfig, but CMUX_UITEST_CHAT_INITIAL_SCROLL does not. Thread through UITestEnvironmentConfig(environment: ProcessInfo.processInfo.environment).chatInitialScroll (adding the new property) to stay consistent with the rest of the PR's established pattern.
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!
Comment on lines
+42
to
+56
| set { super.accessibilityValue = newValue } | ||
| } | ||
| #endif | ||
|
|
||
| override func layoutSubviews() { | ||
| let oldBoundsSize = lastBoundsSize | ||
| let oldContentSize = lastContentSize | ||
| let oldViewport = lastViewport | ||
| super.layoutSubviews() | ||
| lastBoundsSize = bounds.size | ||
| lastContentSize = contentSize | ||
| recordViewport() | ||
| #if DEBUG | ||
| updateDebugAccessibilityValue() | ||
| #endif |
Contributor
There was a problem hiding this comment.
ChatTranscriptUITableView.swift contains ~120 lines of #if DEBUG code — keyboardDebug* stored properties, an accessibilityValue override that returns a formatted debug string, and updateDebugAccessibilityValue() — all of which exist solely to expose scroll/keyboard metrics to UI tests through the accessibility system. This is a test-observable state accessor in a production Sources/ file with no production callers.
By contrast, the keyboard-tracking debug helpers are cleanly isolated in ChatKeyboardTrackingViewController+Debug.swift. Moving the #if DEBUG block in this file to a companion ChatTranscriptUITableView+Debug.swift would match that established pattern and keep the production type's surface clean. The accessibilityValue override in particular changes observable accessibility behavior in debug builds (VoiceOver would speak raw metric strings rather than the table's natural value).
Rule Used: Flag Swift files under a production Sources path (... (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!
Collaborator
Author
|
Superseded by #7072, which carries only the focused scroll-edge fix on current main. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/iOS/CmuxMobileSupport/Sources/CmuxMobileSupport/ComposerDictationController.swift (1)
210-230: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDon't expose the sleep-based graceful stop as public API.
Line 210 widens
stop()across packages, but the implementation still resolves.stoppingvia a fixed 2.5 sTask.sleepwatchdog. That turns an internal timing-repair path into a public contract and lets new callers depend on delay-based state recovery instead of an explicit completion signal. Keepstop()internal or refactor it to a signal-driven async API before widening it. As per coding guidelines, "Do not use ...Task.sleep... in shipped Swift code" and the**/*.swiftpath instruction says to apply the cmux custom Swift lint rules in.github/review-bot-rules/as the source of truth.🤖 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/iOS/CmuxMobileSupport/Sources/CmuxMobileSupport/ComposerDictationController.swift` around lines 210 - 230, The public `stop()` API in `ComposerDictationController` is exposing an internal timing-based cleanup path that relies on `Task.sleep`, which should not be part of shipped Swift behavior. Keep `stop()` internal or change it to an explicit signal-driven async completion flow, and remove the sleep-based watchdog from the `.stopping` recovery path so callers do not depend on fixed-delay state resolution.Sources: Coding guidelines, Path instructions
🤖 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 `@ios/cmuxUITests/cmuxUITests.swift`:
- Around line 616-640: The keyboard-evidence tests in the transcript sampling
flow are still tied to absolute launch-time delays and a fixed sampling window,
which makes them flaky. Update the affected test blocks around
launchAgentChatInlinePreviewApp, waitForTranscriptMetrics, scrollTranscript, and
sampleKeyboardEvidenceFrames so sampling starts only after baseline transcript
metrics are observed and keyboard milestones are reached, using keyboardEvents
or keyboardOverlap as the trigger instead of elapsed wall-clock time. Keep the
screenshots/evidence capture opportunistic during those milestone checks, and
apply the same change to all similar occurrences in the listed test scenarios.
- Around line 1766-1808: The parser in init?(_ rawValue: String) currently
defaults topEdgeEffectSoft and bottomEdgeEffectSoft to false when the probe keys
are missing, which can hide a missing iOS 26 scroll-edge metric. Update this
parsing path so the edge-effect fields are required like the other core probe
values: reject the payload by returning nil if topEdgeEffectSoft or
bottomEdgeEffectSoft is absent, and keep the existing required-field handling
consistent with the other frame/bounds fields.
In
`@Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Composer/ChatComposerDebugAutofocusBridge.swift`:
- Around line 1-102: This DEBUG-only autofocus bridge should not live in the
production Sources target; move ChatComposerDebugAutofocusBridge and its UIView
extension helpers out of the production module into a test-only or debug-only
target. Keep the functionality in a UI-test/debug location instead of
ChatComposerDebugAutofocusBridge.swift under Sources, and update any references
so the production build no longer includes the environment-variable-driven
autofocus/dismiss/refocus behavior.
In
`@Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Composer/ChatComposerView.swift`:
- Around line 77-79: Remove the DEBUG-only autofocus test hook from the
production composer body. In ChatComposerView, eliminate the conditional
background using ChatComposerDebugAutofocusBridge() from the Sources path and
move any UI-test autofocus wiring into the test target or another non-production
harness. Keep ChatComposerView and its body free of `#if` DEBUG test/debug seams
so production Swift sources do not depend on test-only behavior.
- Around line 360-375: `performPaste()` is writing pasted images directly into
`attachments`, while `loadPickedItems(_:)` and the picker flow rebuild state
from `pickedItems` only, so the composer has two attachment sources. Update the
`ChatComposerView` attachment flow so pasted and picked attachments share one
authoritative source of truth, either by merging picker-derived items with
existing non-picker attachments or by routing both paths through the same
attachment model. Make sure the symbols `performPaste()`, `loadPickedItems(_:)`,
`attachments`, and `pickedItems` stay consistent so repicking does not discard
previously pasted items.
In
`@Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Screen/ChatKeyboardTrackingViewController.swift`:
- Around line 303-310: `updateMeasuredGeometryConstants()` is doing an expensive
full subtree scan through `trackedTranscriptTables(in:)` on every
layout/keyboard tick, which makes `ChatKeyboardTrackingViewController` updates
unnecessarily O(subviews) per event. Cache the `ChatTranscriptUITableView` (or
return it from the coordinator) and reuse that reference in
`updateTranscriptOverlayBottomInset(_:)` instead of rescanning, then only
refresh the cache when the hosted transcript hierarchy changes. Apply the same
fix to the other repeated scan call sites in this controller that drive the
keyboard/layout update path.
In
`@Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Screen/ChatKeyboardTrackingViewController`+Debug.swift:
- Around line 1-73: The DEBUG-only keyboard geometry instrumentation in
ChatKeyboardTrackingViewController+Debug should not live under the production
Sources target. Move the updateKeyboardDebugValues(overlap:),
clampedKeyboardAnimationProgress(overlap:), and related keyboardDebug… plumbing
into a test-only target or debug support module, or reduce it to the minimum
internal state needed for `@testable` import. Keep
ChatKeyboardTrackingViewController free of test/debug seams in production Swift
source.
In
`@Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptTableView.swift`:
- Around line 85-87: Remove the UI-test-only initial-scroll/debug seam from
ChatTranscriptTableView and keep production transcript coordination free of test
orchestration. Delete the didApplyDebugInitialScroll state and the env-driven
branch from the coordinator path, and move any initial-scroll handling plus the
extra debug accessibility update into the UI test harness or a dedicated
test-only helper outside Sources. Update ChatTranscriptTableView and any related
methods involved in the initial scroll flow so only production behavior remains.
In
`@Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptUITableView.swift`:
- Around line 12-27: The DEBUG-only transcript metrics hook is currently
embedded in the production ChatTranscriptUITableView source, including the
keyboardDebug* fields and the accessibilityValue exporter. Move these seams into
a dedicated test/debug support target or extension outside Sources, and keep
ChatTranscriptUITableView in shipping code free of UI-test-only geometry hooks.
Update the related accessibilityValue and metric plumbing together so the
production type no longer exposes test/debug-only state.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/AgentChatDemoScreen.swift`:
- Around line 74-83: The icon-only toolbar buttons in AgentChatDemoScreen’s
ToolbarItemGroup need user-facing accessibility handling, since identifiers
alone are not enough for VoiceOver. Update each Button to either provide a
localized accessibilityLabel using the existing accessibilityIdentifier targets,
or mark them as hidden from accessibility if they are only preview chrome. Keep
the fix localized and consistent with the app’s SwiftUI internationalization
rules.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceChatPane.swift`:
- Around line 182-194: The .custom shortcut path in WorkspaceChatPane is lossy
because it converts custom.output to UTF-8 text before sending, which can drop
or alter non-UTF-8 bytes. Update the shortcut handling in the .custom(custom)
branch and any related paths in WorkspaceChatPane to preserve the original
payload bytes end-to-end instead of decoding to String. Use the existing
custom.output and sendSessionTerminalInput flow, but pass the raw data through a
bytes-preserving API or equivalent so shared shortcuts behave the same in chat
and terminal.
In
`@Packages/iOS/CmuxMobileSupport/Sources/CmuxMobileSupport/ComposerDictationTextMerger.swift`:
- Around line 41-59: The transition guard properties on the state type are
exposing internal state-machine rules as part of the public API. Make
`canStart`, `canCancelPendingStart`, `canFinalize`, and `isStopping`
internal/private in `ComposerDictationTextMerger` (or the relevant state
enum/class), and keep only the externally meaningful read models like
`isListening` and `locksComposerField` public so downstream code cannot depend
on these implementation details.
In
`@Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift`:
- Around line 873-878: The test helper setKeyboardHeightForTesting(_:) is only
updating keyboardHeight, leaving the new keyboardVisible source of truth out of
sync with the injected state. Update this helper so it also derives and sets
keyboardVisible consistently from the passed height before calling
layoutRenderedTerminalForCurrentViewport(), layoutBottomDock(), and
syncSurfaceGeometry(shouldReassertNaturalSize:), matching the runtime behavior
used by keyboardUp checks and hide/refocus logic elsewhere in
GhosttySurfaceView.
---
Outside diff comments:
In
`@Packages/iOS/CmuxMobileSupport/Sources/CmuxMobileSupport/ComposerDictationController.swift`:
- Around line 210-230: The public `stop()` API in `ComposerDictationController`
is exposing an internal timing-based cleanup path that relies on `Task.sleep`,
which should not be part of shipped Swift behavior. Keep `stop()` internal or
change it to an explicit signal-driven async completion flow, and remove the
sleep-based watchdog from the `.stopping` recovery path so callers do not depend
on fixed-delay state resolution.
🪄 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: 9196ddbd-133b-45a9-9aa5-577b2d804768
📒 Files selected for processing (54)
Packages/iOS/CmuxAgentChatUI/Package.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Composer/ChatAccessoryChipRow.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Composer/ChatAccessoryShortcut.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Composer/ChatAccessoryShortcutSemanticAction.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Composer/ChatComposerDebugAutofocusBridge.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Composer/ChatComposerMaterialBackground.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Composer/ChatComposerPasteboard.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Composer/ChatComposerView.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Screen/ChatKeyboardNotificationToken.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Screen/ChatKeyboardTrackedRoot.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Screen/ChatKeyboardTrackingContainer.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Screen/ChatKeyboardTrackingViewController+Debug.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Screen/ChatKeyboardTrackingViewController.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Screen/ChatScreen.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptListView.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptTableView.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptUITableView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/AgentChatDemoScreen.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/AgentChatDemoScreenStyle.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView+AgentChatPreview.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GlassInputPill.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalAccessoryChatCompatibility.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalComposerView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalShortcutsSettingsView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceChatPane.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/iOS/CmuxMobileSupport/Sources/CmuxMobileSupport/ComposerDictationController.swiftPackages/iOS/CmuxMobileSupport/Sources/CmuxMobileSupport/ComposerDictationTextMerger.swiftPackages/iOS/CmuxMobileSupport/Sources/CmuxMobileSupport/MobileComposerFieldContainer.swiftPackages/iOS/CmuxMobileSupport/Sources/CmuxMobileSupport/MobileComposerIconButton.swiftPackages/iOS/CmuxMobileSupport/Sources/CmuxMobileSupport/MobileComposerIconLabel.swiftPackages/iOS/CmuxMobileSupport/Sources/CmuxMobileSupport/MobileKeyboardReservation.swiftPackages/iOS/CmuxMobileSupport/Sources/CmuxMobileSupport/MobileKeyboardTransition.swiftPackages/iOS/CmuxMobileSupport/Sources/CmuxMobileSupport/MobileKeyboardVisibility.swiftPackages/iOS/CmuxMobileSupport/Sources/CmuxMobileSupport/MobileScrollViewportSnapshot.swiftPackages/iOS/CmuxMobileSupport/Sources/CmuxMobileSupport/UITestConfig.swiftPackages/iOS/CmuxMobileSupport/Sources/CmuxMobileSupport/UITestEnvironmentConfig.swiftPackages/iOS/CmuxMobileSupport/Sources/CmuxMobileSupport/View+MobileGlass.swiftPackages/iOS/CmuxMobileSupport/Tests/CmuxMobileSupportTests/ComposerDictationTests.swiftPackages/iOS/CmuxMobileSupport/Tests/CmuxMobileSupportTests/MobileKeyboardTrackingTests.swiftPackages/iOS/CmuxMobileSupport/Tests/CmuxMobileSupportTests/UITestConfigTests.swiftPackages/iOS/CmuxMobileTerminal/Package.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/TerminalAccessoryConfiguration.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/TerminalArrowNubDirection.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/TerminalArrowNubView.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/TerminalKeyboardHeightAnimation.swiftios/cmux/CmuxAppDelegate.swiftios/cmux/Resources/Localizable.xcstringsios/cmuxPackage/Sources/cmuxFeature/MobileAnalyticsComposition.swiftios/cmuxPackage/Sources/cmuxFeature/MobileFeedbackStamp+Current.swiftios/cmuxUITests/cmuxUITests.swiftscripts/check-package-resolved-policy.py
💤 Files with no reviewable changes (1)
- Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift
Comment on lines
+616
to
+640
| let app = launchAgentChatInlinePreviewApp(environment: [ | ||
| "CMUX_UITEST_CHAT_AUTOFOCUS_DELAY": "14.0", | ||
| "CMUX_UITEST_CHAT_AUTO_DISMISS_DELAY": "1.25", | ||
| ]) | ||
| let table = app.tables["ChatTranscriptTableView"] | ||
| XCTAssertTrue(table.waitForExistence(timeout: 8)) | ||
| let composerBar = app.otherElements["ChatComposerBar"] | ||
| XCTAssertTrue(composerBar.waitForExistence(timeout: 8)) | ||
|
|
||
| let loadedMetrics = try waitForTranscriptMetrics(table, timeout: 8) { | ||
| $0.frameHeight > 240 && $0.frameMaxY > 300 && $0.contentHeight > $0.boundsHeight * 1.6 | ||
| } | ||
| try scrollTranscript(table, direction: .down, timeout: 5) { | ||
| $0.distanceFromBottom > 180 && $0.offsetY > 100 | ||
| } | ||
| let beforeKeyboard = try waitForTranscriptMetrics(table, timeout: 2) { | ||
| abs($0.frameMaxY - loadedMetrics.frameMaxY) < 4 && $0.keyboardOverlap == 0 | ||
| } | ||
|
|
||
| let animationSamples = sampleKeyboardEvidenceFrames( | ||
| table: table, | ||
| composerBar: composerBar, | ||
| duration: 8.0, | ||
| frameCapturePrefix: "middle" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make keyboard-evidence sampling milestone-driven instead of launch-clock driven.
These tests arm autofocus/dismiss/refocus with absolute delays like 14.0 after launch, then sample for a fixed window from wherever setup happens to finish. Fast CI can finish the 8s sample before autofocus fires; slow CI can start after the transition. Trigger actions after baseline metrics are observed, or wait on keyboardEvents / keyboardOverlap milestones and attach screenshots opportunistically. As per coding guidelines, tests must not depend on real wall-clock time and must assert on causality, not latency.
Also applies to: 697-721, 768-793, 844-869, 1888-1919
🤖 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 `@ios/cmuxUITests/cmuxUITests.swift` around lines 616 - 640, The
keyboard-evidence tests in the transcript sampling flow are still tied to
absolute launch-time delays and a fixed sampling window, which makes them flaky.
Update the affected test blocks around launchAgentChatInlinePreviewApp,
waitForTranscriptMetrics, scrollTranscript, and sampleKeyboardEvidenceFrames so
sampling starts only after baseline transcript metrics are observed and keyboard
milestones are reached, using keyboardEvents or keyboardOverlap as the trigger
instead of elapsed wall-clock time. Keep the screenshots/evidence capture
opportunistic during those milestone checks, and apply the same change to all
similar occurrences in the listed test scenarios.
Source: Coding guidelines
Comment on lines
+1766
to
+1808
| init?(_ rawValue: String) { | ||
| var values: [String: CGFloat] = [:] | ||
| for pair in rawValue.split(separator: ";") { | ||
| let parts = pair.split(separator: "=", maxSplits: 1) | ||
| guard parts.count == 2, | ||
| let value = Double(parts[1]) else { | ||
| continue | ||
| } | ||
| values[String(parts[0])] = CGFloat(value) | ||
| } | ||
| guard let frameMinY = values["frameMinY"], | ||
| let frameMaxY = values["frameMaxY"], | ||
| let frameHeight = values["frameHeight"], | ||
| let boundsHeight = values["boundsHeight"], | ||
| let offsetY = values["offsetY"], | ||
| let visibleBottomY = values["visibleBottomY"], | ||
| let contentHeight = values["contentHeight"], | ||
| let distanceFromBottom = values["distanceFromBottom"] else { | ||
| return nil | ||
| } | ||
| self.frameMinY = frameMinY | ||
| self.frameMaxY = frameMaxY | ||
| self.frameHeight = frameHeight | ||
| self.presentationFrameMaxY = values["presentationFrameMaxY"] ?? frameMaxY | ||
| self.boundsHeight = boundsHeight | ||
| self.offsetY = offsetY | ||
| self.visibleBottomY = visibleBottomY | ||
| self.contentHeight = contentHeight | ||
| self.distanceFromBottom = distanceFromBottom | ||
| self.keyboardEvents = Int(values["keyboardEvents"] ?? 0) | ||
| self.keyboardOverlap = values["keyboardOverlap"] ?? 0 | ||
| self.keyboardTargetOverlap = values["keyboardTargetOverlap"] ?? self.keyboardOverlap | ||
| self.composerMinY = values["composerMinY"] ?? frameMaxY | ||
| self.composerPresentationMinY = values["composerPresentationMinY"] ?? self.composerMinY | ||
| self.presentationGap = values["presentationGap"] ?? 0 | ||
| self.composerOverlayBottomInset = values["composerOverlayBottomInset"] ?? 0 | ||
| self.keyboardAnimationActive = (values["keyboardAnimationActive"] ?? 0) >= 0.5 | ||
| self.keyboardAnimationProgress = values["keyboardAnimationProgress"] ?? 1 | ||
| self.keyboardTransitionDuration = TimeInterval(values["keyboardTransitionDuration"] ?? 0) | ||
| self.maxAnimationPresentationGap = values["maxAnimationPresentationGap"] ?? 0 | ||
| self.keyboardAnimationSamples = Int(values["keyboardAnimationSamples"] ?? 0) | ||
| self.topEdgeEffectSoft = (values["topEdgeEffectSoft"] ?? 0) >= 0.5 | ||
| self.bottomEdgeEffectSoft = (values["bottomEdgeEffectSoft"] ?? 0) >= 0.5 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require scroll-edge probe fields instead of defaulting missing values to false.
Line 1807 makes topEdgeEffectSoft false when the probe key is absent, so the Line 1050 regression assertion can pass even if the app stops publishing the iOS 26 top-edge metric. Make the edge-effect keys required for this parser.
Proposed fix
- let visibleBottomY = values["visibleBottomY"],
- let contentHeight = values["contentHeight"],
- let distanceFromBottom = values["distanceFromBottom"] else {
+ let visibleBottomY = values["visibleBottomY"],
+ let contentHeight = values["contentHeight"],
+ let distanceFromBottom = values["distanceFromBottom"],
+ let topEdgeEffectSoft = values["topEdgeEffectSoft"],
+ let bottomEdgeEffectSoft = values["bottomEdgeEffectSoft"] else {
return nil
}
@@
- self.topEdgeEffectSoft = (values["topEdgeEffectSoft"] ?? 0) >= 0.5
- self.bottomEdgeEffectSoft = (values["bottomEdgeEffectSoft"] ?? 0) >= 0.5
+ self.topEdgeEffectSoft = topEdgeEffectSoft >= 0.5
+ self.bottomEdgeEffectSoft = bottomEdgeEffectSoft >= 0.5📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| init?(_ rawValue: String) { | |
| var values: [String: CGFloat] = [:] | |
| for pair in rawValue.split(separator: ";") { | |
| let parts = pair.split(separator: "=", maxSplits: 1) | |
| guard parts.count == 2, | |
| let value = Double(parts[1]) else { | |
| continue | |
| } | |
| values[String(parts[0])] = CGFloat(value) | |
| } | |
| guard let frameMinY = values["frameMinY"], | |
| let frameMaxY = values["frameMaxY"], | |
| let frameHeight = values["frameHeight"], | |
| let boundsHeight = values["boundsHeight"], | |
| let offsetY = values["offsetY"], | |
| let visibleBottomY = values["visibleBottomY"], | |
| let contentHeight = values["contentHeight"], | |
| let distanceFromBottom = values["distanceFromBottom"] else { | |
| return nil | |
| } | |
| self.frameMinY = frameMinY | |
| self.frameMaxY = frameMaxY | |
| self.frameHeight = frameHeight | |
| self.presentationFrameMaxY = values["presentationFrameMaxY"] ?? frameMaxY | |
| self.boundsHeight = boundsHeight | |
| self.offsetY = offsetY | |
| self.visibleBottomY = visibleBottomY | |
| self.contentHeight = contentHeight | |
| self.distanceFromBottom = distanceFromBottom | |
| self.keyboardEvents = Int(values["keyboardEvents"] ?? 0) | |
| self.keyboardOverlap = values["keyboardOverlap"] ?? 0 | |
| self.keyboardTargetOverlap = values["keyboardTargetOverlap"] ?? self.keyboardOverlap | |
| self.composerMinY = values["composerMinY"] ?? frameMaxY | |
| self.composerPresentationMinY = values["composerPresentationMinY"] ?? self.composerMinY | |
| self.presentationGap = values["presentationGap"] ?? 0 | |
| self.composerOverlayBottomInset = values["composerOverlayBottomInset"] ?? 0 | |
| self.keyboardAnimationActive = (values["keyboardAnimationActive"] ?? 0) >= 0.5 | |
| self.keyboardAnimationProgress = values["keyboardAnimationProgress"] ?? 1 | |
| self.keyboardTransitionDuration = TimeInterval(values["keyboardTransitionDuration"] ?? 0) | |
| self.maxAnimationPresentationGap = values["maxAnimationPresentationGap"] ?? 0 | |
| self.keyboardAnimationSamples = Int(values["keyboardAnimationSamples"] ?? 0) | |
| self.topEdgeEffectSoft = (values["topEdgeEffectSoft"] ?? 0) >= 0.5 | |
| self.bottomEdgeEffectSoft = (values["bottomEdgeEffectSoft"] ?? 0) >= 0.5 | |
| init?(_ rawValue: String) { | |
| var values: [String: CGFloat] = [:] | |
| for pair in rawValue.split(separator: ";") { | |
| let parts = pair.split(separator: "=", maxSplits: 1) | |
| guard parts.count == 2, | |
| let value = Double(parts[1]) else { | |
| continue | |
| } | |
| values[String(parts[0])] = CGFloat(value) | |
| } | |
| guard let frameMinY = values["frameMinY"], | |
| let frameMaxY = values["frameMaxY"], | |
| let frameHeight = values["frameHeight"], | |
| let boundsHeight = values["boundsHeight"], | |
| let offsetY = values["offsetY"], | |
| let visibleBottomY = values["visibleBottomY"], | |
| let contentHeight = values["contentHeight"], | |
| let distanceFromBottom = values["distanceFromBottom"], | |
| let topEdgeEffectSoft = values["topEdgeEffectSoft"], | |
| let bottomEdgeEffectSoft = values["bottomEdgeEffectSoft"] else { | |
| return nil | |
| } | |
| self.frameMinY = frameMinY | |
| self.frameMaxY = frameMaxY | |
| self.frameHeight = frameHeight | |
| self.presentationFrameMaxY = values["presentationFrameMaxY"] ?? frameMaxY | |
| self.boundsHeight = boundsHeight | |
| self.offsetY = offsetY | |
| self.visibleBottomY = visibleBottomY | |
| self.contentHeight = contentHeight | |
| self.distanceFromBottom = distanceFromBottom | |
| self.keyboardEvents = Int(values["keyboardEvents"] ?? 0) | |
| self.keyboardOverlap = values["keyboardOverlap"] ?? 0 | |
| self.keyboardTargetOverlap = values["keyboardTargetOverlap"] ?? self.keyboardOverlap | |
| self.composerMinY = values["composerMinY"] ?? frameMaxY | |
| self.composerPresentationMinY = values["composerPresentationMinY"] ?? self.composerMinY | |
| self.presentationGap = values["presentationGap"] ?? 0 | |
| self.composerOverlayBottomInset = values["composerOverlayBottomInset"] ?? 0 | |
| self.keyboardAnimationActive = (values["keyboardAnimationActive"] ?? 0) >= 0.5 | |
| self.keyboardAnimationProgress = values["keyboardAnimationProgress"] ?? 1 | |
| self.keyboardTransitionDuration = TimeInterval(values["keyboardTransitionDuration"] ?? 0) | |
| self.maxAnimationPresentationGap = values["maxAnimationPresentationGap"] ?? 0 | |
| self.keyboardAnimationSamples = Int(values["keyboardAnimationSamples"] ?? 0) | |
| self.topEdgeEffectSoft = topEdgeEffectSoft >= 0.5 | |
| self.bottomEdgeEffectSoft = bottomEdgeEffectSoft >= 0.5 |
🤖 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 `@ios/cmuxUITests/cmuxUITests.swift` around lines 1766 - 1808, The parser in
init?(_ rawValue: String) currently defaults topEdgeEffectSoft and
bottomEdgeEffectSoft to false when the probe keys are missing, which can hide a
missing iOS 26 scroll-edge metric. Update this parsing path so the edge-effect
fields are required like the other core probe values: reject the payload by
returning nil if topEdgeEffectSoft or bottomEdgeEffectSoft is absent, and keep
the existing required-field handling consistent with the other frame/bounds
fields.
Comment on lines
+1
to
+102
| #if DEBUG && os(iOS) | ||
| import Foundation | ||
| import SwiftUI | ||
| import UIKit | ||
|
|
||
| struct ChatComposerDebugAutofocusBridge: UIViewRepresentable { | ||
| func makeUIView(context: Context) -> UIView { | ||
| let view = UIView(frame: .zero) | ||
| view.isUserInteractionEnabled = false | ||
| view.isAccessibilityElement = false | ||
| view.accessibilityElementsHidden = true | ||
| scheduleChatComposerDebugAutofocus(from: view) | ||
| return view | ||
| } | ||
|
|
||
| func updateUIView(_ uiView: UIView, context: Context) {} | ||
|
|
||
| @MainActor | ||
| private func scheduleChatComposerDebugAutofocus(from view: UIView) { | ||
| guard let delay = chatComposerDebugTimeInterval("CMUX_UITEST_CHAT_AUTOFOCUS_DELAY") else { | ||
| return | ||
| } | ||
| UIView.animate(withDuration: 0, delay: max(0, delay), options: [.allowUserInteraction]) { | ||
| } completion: { _ in | ||
| MainActor.assumeIsolated { | ||
| let root = view.window ?? view.cmuxRootView() | ||
| let input = root.cmuxFirstFocusableTextInput(preferredIdentifier: "ChatComposerField") | ||
| _ = input?.becomeFirstResponder() | ||
| scheduleChatComposerDebugDismissAndRefocus(for: input) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @MainActor | ||
| private func scheduleChatComposerDebugDismissAndRefocus(for input: UIView?) { | ||
| guard let autoDismissDelay = chatComposerDebugTimeInterval("CMUX_UITEST_CHAT_AUTO_DISMISS_DELAY") else { | ||
| return | ||
| } | ||
| UIView.animate(withDuration: 0, delay: max(0, autoDismissDelay), options: [.allowUserInteraction]) { | ||
| } completion: { _ in | ||
| MainActor.assumeIsolated { | ||
| input?.resignFirstResponder() | ||
| guard let autoRefocusDelay = chatComposerDebugTimeInterval("CMUX_UITEST_CHAT_AUTO_REFOCUS_AFTER_DISMISS_DELAY") else { | ||
| return | ||
| } | ||
| UIView.animate(withDuration: 0, delay: max(0, autoRefocusDelay), options: [.allowUserInteraction]) { | ||
| } completion: { _ in | ||
| MainActor.assumeIsolated { | ||
| _ = input?.becomeFirstResponder() | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private func chatComposerDebugTimeInterval(_ name: String) -> TimeInterval? { | ||
| guard let raw = ProcessInfo.processInfo.environment[name]? | ||
| .trimmingCharacters(in: .whitespacesAndNewlines), | ||
| !raw.isEmpty, | ||
| let value = Double(raw) | ||
| else { | ||
| return nil | ||
| } | ||
| return value | ||
| } | ||
| } | ||
|
|
||
| private extension UIView { | ||
| @MainActor | ||
| func cmuxRootView() -> UIView { | ||
| var current = self | ||
| while let superview = current.superview { | ||
| current = superview | ||
| } | ||
| return current | ||
| } | ||
|
|
||
| @MainActor | ||
| func cmuxFirstFocusableTextInput(preferredIdentifier: String) -> UIView? { | ||
| if (self is UITextField || self is UITextView), canBecomeFirstResponder { | ||
| if accessibilityIdentifier == preferredIdentifier { | ||
| return self | ||
| } | ||
| } | ||
| for subview in subviews { | ||
| if let found = subview.cmuxFirstFocusableTextInput(preferredIdentifier: preferredIdentifier), | ||
| found.accessibilityIdentifier == preferredIdentifier { | ||
| return found | ||
| } | ||
| } | ||
| if (self is UITextField || self is UITextView), canBecomeFirstResponder { | ||
| return self | ||
| } | ||
| for subview in subviews { | ||
| if let found = subview.cmuxFirstFocusableTextInput(preferredIdentifier: preferredIdentifier) { | ||
| return found | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
| } | ||
| #endif |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Move the autofocus bridge out of the production target.
This file is entirely DEBUG-only UI-test scaffolding, and it drives focus/dismiss/refocus with wall-clock delays from environment variables. That both adds a test seam under Sources/ and keeps the UI test path timing-sensitive. As per coding guidelines, "Do not add test-only or debug-only seams in production Swift source files" and "Tests must not depend on real wall-clock time." As per path instructions, **/Sources/**/*.swift applies .github/review-bot-rules/no-test-debug-seam-in-production-source.md.
🤖 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/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Composer/ChatComposerDebugAutofocusBridge.swift`
around lines 1 - 102, This DEBUG-only autofocus bridge should not live in the
production Sources target; move ChatComposerDebugAutofocusBridge and its UIView
extension helpers out of the production module into a test-only or debug-only
target. Keep the functionality in a UI-test/debug location instead of
ChatComposerDebugAutofocusBridge.swift under Sources, and update any references
so the production build no longer includes the environment-variable-driven
autofocus/dismiss/refocus behavior.
Sources: Coding guidelines, Path instructions
Comment on lines
+77
to
+79
| #if DEBUG | ||
| .background(ChatComposerDebugAutofocusBridge()) | ||
| #endif |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the UI-test autofocus hook from production Sources/ code.
ChatComposerDebugAutofocusBridge() adds a DEBUG-only test seam directly to the composer body. This repo explicitly forbids #if DEBUG test/debug hooks in production Swift sources; keep that wiring in the test target or another non-production harness instead. As per coding guidelines, production Swift source must not add test-only or debug-only seams, and as per path instructions, Sources/**/*.swift should not gain #if DEBUG members used only for tests/debugging.
🤖 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/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Composer/ChatComposerView.swift`
around lines 77 - 79, Remove the DEBUG-only autofocus test hook from the
production composer body. In ChatComposerView, eliminate the conditional
background using ChatComposerDebugAutofocusBridge() from the Sources path and
move any UI-test autofocus wiring into the test target or another non-production
harness. Keep ChatComposerView and its body free of `#if` DEBUG test/debug seams
so production Swift sources do not depend on test-only behavior.
Sources: Coding guidelines, Path instructions
Comment on lines
+360
to
+375
| private func performPaste() { | ||
| let pasteboard = UIPasteboard.general | ||
| if attachments.count < 4, | ||
| let attachment = pasteboard.chatComposerAttachment( | ||
| maxDimension: Self.maxAttachmentDimension, | ||
| jpegQuality: Self.jpegQuality | ||
| ) { | ||
| attachments.append(attachment) | ||
| isDraftFocused = true | ||
| return | ||
| } | ||
| guard let string = pasteboard.chatComposerText() else { | ||
| return | ||
| } | ||
| draft += string | ||
| isDraftFocused = true |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep pasted and picked attachments on one authoritative path.
performPaste() appends image attachments straight into attachments, but the picker flow still rebuilds the draft from pickedItems only in loadPickedItems(_:). After pasting an image, picking or re-picking a photo will drop the pasted attachment from the composer. Merge picker-derived attachments into the existing non-picker items, or move both entry points onto one attachment source of truth.
🤖 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/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Composer/ChatComposerView.swift`
around lines 360 - 375, `performPaste()` is writing pasted images directly into
`attachments`, while `loadPickedItems(_:)` and the picker flow rebuild state
from `pickedItems` only, so the composer has two attachment sources. Update the
`ChatComposerView` attachment flow so pasted and picked attachments share one
authoritative source of truth, either by merging picker-derived items with
existing non-picker attachments or by routing both paths through the same
attachment model. Make sure the symbols `performPaste()`, `loadPickedItems(_:)`,
`attachments`, and `pickedItems` stay consistent so repicking does not discard
previously pasted items.
Comment on lines
+12
to
+27
| #if DEBUG | ||
| var keyboardDebugEventCount = 0 | ||
| var keyboardDebugOverlap: CGFloat = 0 | ||
| var keyboardDebugTargetOverlap: CGFloat = 0 | ||
| var keyboardDebugGuideOverlap: CGFloat = 0 | ||
| var keyboardDebugBottomConstraint: CGFloat = 0 | ||
| var keyboardDebugComposerMinY: CGFloat = 0 | ||
| var keyboardDebugComposerPresentationMinY: CGFloat = 0 | ||
| var keyboardDebugPresentationFrameMaxY: CGFloat = 0 | ||
| var keyboardDebugPresentationFrameMaxYProvider: (() -> CGFloat?)? | ||
| var keyboardDebugComposerPresentationMinYProvider: (() -> CGFloat?)? | ||
| var keyboardDebugAnimationID = 0 | ||
| var keyboardDebugAnimationActive = false | ||
| var keyboardDebugAnimationProgress: CGFloat = 1 | ||
| var keyboardDebugTransitionDuration: TimeInterval = 0 | ||
| #endif |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Move the transcript metrics hook out of Sources/.
These DEBUG-only fields and the accessibilityValue exporter exist to feed UI-test geometry assertions, but they now live on the production ChatTranscriptUITableView type. Keep this in a dedicated test/debug support target instead of the shipping package source. As per coding guidelines, "Do not add test-only or debug-only seams in production Swift source files." As per path instructions, **/Sources/**/*.swift applies .github/review-bot-rules/no-test-debug-seam-in-production-source.md.
Also applies to: 39-44, 120-202
🤖 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/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptUITableView.swift`
around lines 12 - 27, The DEBUG-only transcript metrics hook is currently
embedded in the production ChatTranscriptUITableView source, including the
keyboardDebug* fields and the accessibilityValue exporter. Move these seams into
a dedicated test/debug support target or extension outside Sources, and keep
ChatTranscriptUITableView in shipping code free of UI-test-only geometry hooks.
Update the related accessibilityValue and metric plumbing together so the
production type no longer exposes test/debug-only state.
Sources: Coding guidelines, Path instructions
Comment on lines
+74
to
+83
| ToolbarItemGroup(placement: .topBarTrailing) { | ||
| Button(action: {}) { | ||
| Image(systemName: "bubble.left.and.bubble.right.fill") | ||
| } | ||
| .accessibilityIdentifier("AgentChatInlinePreviewChatToggle") | ||
| Button(action: {}) { | ||
| Image(systemName: "rectangle.stack") | ||
| } | ||
| .accessibilityIdentifier("AgentChatInlinePreviewTerminalPicker") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add localized accessibility labels to the new icon-only toolbar buttons.
These controls are currently discoverable only by identifier; VoiceOver will not get a user-facing name. If they are meant to be interactive, give each button a localized accessibilityLabel. If they are purely preview chrome, hide them from accessibility instead. As per coding guidelines, all user-facing Swift UI text must be localized, and path instructions apply the full-internationalization rule to Swift UI changes.
Suggested fix
ToolbarItemGroup(placement: .topBarTrailing) {
Button(action: {}) {
Image(systemName: "bubble.left.and.bubble.right.fill")
}
.accessibilityIdentifier("AgentChatInlinePreviewChatToggle")
+ .accessibilityLabel(
+ String(localized: "agentChatDemo.inline.chatToggle", defaultValue: "Toggle chat")
+ )
Button(action: {}) {
Image(systemName: "rectangle.stack")
}
.accessibilityIdentifier("AgentChatInlinePreviewTerminalPicker")
+ .accessibilityLabel(
+ String(localized: "agentChatDemo.inline.terminalPicker", defaultValue: "Show terminal picker")
+ )
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ToolbarItemGroup(placement: .topBarTrailing) { | |
| Button(action: {}) { | |
| Image(systemName: "bubble.left.and.bubble.right.fill") | |
| } | |
| .accessibilityIdentifier("AgentChatInlinePreviewChatToggle") | |
| Button(action: {}) { | |
| Image(systemName: "rectangle.stack") | |
| } | |
| .accessibilityIdentifier("AgentChatInlinePreviewTerminalPicker") | |
| } | |
| ToolbarItemGroup(placement: .topBarTrailing) { | |
| Button(action: {}) { | |
| Image(systemName: "bubble.left.and.bubble.right.fill") | |
| } | |
| .accessibilityIdentifier("AgentChatInlinePreviewChatToggle") | |
| .accessibilityLabel( | |
| String(localized: "agentChatDemo.inline.chatToggle", defaultValue: "Toggle chat") | |
| ) | |
| Button(action: {}) { | |
| Image(systemName: "rectangle.stack") | |
| } | |
| .accessibilityIdentifier("AgentChatInlinePreviewTerminalPicker") | |
| .accessibilityLabel( | |
| String(localized: "agentChatDemo.inline.terminalPicker", defaultValue: "Show terminal picker") | |
| ) | |
| } |
🤖 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/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/AgentChatDemoScreen.swift`
around lines 74 - 83, The icon-only toolbar buttons in AgentChatDemoScreen’s
ToolbarItemGroup need user-facing accessibility handling, since identifiers
alone are not enough for VoiceOver. Update each Button to either provide a
localized accessibilityLabel using the existing accessibilityIdentifier targets,
or mark them as hidden from accessibility if they are only preview chrome. Keep
the fix localized and consistent with the app’s SwiftUI internationalization
rules.
Sources: Coding guidelines, Path instructions
Comment on lines
+182
to
+194
| case let .custom(custom): | ||
| guard let output = custom.output, | ||
| let text = String(data: output, encoding: .utf8) else { | ||
| return nil | ||
| } | ||
| return ChatAccessoryShortcut( | ||
| id: "terminal.inputAccessory.custom.\(custom.id.uuidString)", | ||
| title: custom.title, | ||
| systemImage: validSymbolName(custom.symbolName), | ||
| accessibilityLabel: custom.title | ||
| ) { | ||
| sendSessionTerminalInput(text) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve shortcut payload bytes instead of round-tripping through UTF-8.
CustomToolbarAction.isSupportedInAgentChat accepts any non-nil output, but this path drops non-UTF-8 payloads and re-encodes the rest before sending. That means a shortcut can be configurable as “shared” yet disappear or send different bytes in chat than it does in the terminal.
Proposed fix
- case let .custom(custom):
- guard let output = custom.output,
- let text = String(data: output, encoding: .utf8) else {
+ case let .custom(custom):
+ guard let output = custom.output else {
return nil
}
return ChatAccessoryShortcut(
id: "terminal.inputAccessory.custom.\(custom.id.uuidString)",
title: custom.title,
systemImage: validSymbolName(custom.symbolName),
accessibilityLabel: custom.title
) {
- sendSessionTerminalInput(text)
+ sendSessionTerminalInput(output)
}
}
}
@@
case .paste:
break
default:
- guard let output = action.output,
- let text = String(data: output, encoding: .utf8) else {
+ guard let output = action.output else {
return
}
- sendSessionTerminalInput(text)
+ sendSessionTerminalInput(output)
}
}
- private func sendSessionTerminalInput(_ text: String) {
- guard let terminalID = session.terminalID,
- let data = text.data(using: .utf8)
- else { return }
+ private func sendSessionTerminalInput(_ data: Data) {
+ guard let terminalID = session.terminalID else { return }
Task {
await store.submitTerminalRawInput(data, surfaceID: terminalID)
}
}Also applies to: 198-224
🤖 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/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceChatPane.swift`
around lines 182 - 194, The .custom shortcut path in WorkspaceChatPane is lossy
because it converts custom.output to UTF-8 text before sending, which can drop
or alter non-UTF-8 bytes. Update the shortcut handling in the .custom(custom)
branch and any related paths in WorkspaceChatPane to preserve the original
payload bytes end-to-end instead of decoding to String. Use the existing
custom.output and sendSessionTerminalInput flow, but pass the raw data through a
bytes-preserving API or equivalent so shared shortcuts behave the same in chat
and terminal.
Comment on lines
+41
to
+59
| public var canStart: Bool { self == .idle } | ||
|
|
||
| /// Whether a tap should cancel a start whose authorization is still resolving. | ||
| /// True only in ``requestingPermission``: a second tap there aborts the pending | ||
| /// start and returns to idle so recognition never begins, rather than being | ||
| /// ignored and letting the mic come on anyway when permission later resolves. | ||
| var canCancelPendingStart: Bool { self == .requestingPermission } | ||
| public var canCancelPendingStart: Bool { self == .requestingPermission } | ||
|
|
||
| /// Whether a graceful stop can finalize from this state. True only while | ||
| /// ``listening``: a graceful stop flushes buffered audio and waits for the | ||
| /// recognition task's final result. From any other state there is no live | ||
| /// session to finalize, so the controller hard-cancels instead. | ||
| var canFinalize: Bool { self == .listening } | ||
| public var canFinalize: Bool { self == .listening } | ||
|
|
||
| /// Whether the controller is mid-teardown, waiting for the recognition task's | ||
| /// final result before returning to idle. The mic is no longer capturing but | ||
| /// the session has not fully settled. Distinguishes a graceful stop's transient | ||
| /// wait from both the active ``listening`` state and the resting ``idle``. | ||
| var isStopping: Bool { self == .stopping } | ||
| public var isStopping: Bool { self == .stopping } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Keep the transition guards internal.
Lines 41-59 expose canStart, canCancelPendingStart, canFinalize, and isStopping, but those are controller-internal transition rules rather than stable UI state. Once public, downstream packages can branch on the current state-machine implementation and lock this package into those exact transitions. Only the externally meaningful read models (isListening, locksComposerField) need to be public here.
🤖 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/iOS/CmuxMobileSupport/Sources/CmuxMobileSupport/ComposerDictationTextMerger.swift`
around lines 41 - 59, The transition guard properties on the state type are
exposing internal state-machine rules as part of the public API. Make
`canStart`, `canCancelPendingStart`, `canFinalize`, and `isStopping`
internal/private in `ComposerDictationTextMerger` (or the relevant state
enum/class), and keep only the externally meaningful read models like
`isListening` and `locksComposerField` public so downstream code cannot depend
on these implementation details.
Comment on lines
873
to
878
| func setKeyboardHeightForTesting(_ height: CGFloat) { | ||
| stopKeyboardHeightAnimation() | ||
| keyboardHeight = max(0, height) | ||
| layoutRenderedTerminalForCurrentViewport() | ||
| layoutBottomDock() | ||
| syncSurfaceGeometry(shouldReassertNaturalSize: true) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the test keyboard helper’s visibility state in sync.
Lines 874-877 only update keyboardHeight, but this refactor moved the behavioral checks and debug probe over to keyboardVisible (for example Lines 752, 963, 1505, and 1584). Any caller that injects a nonzero test height now gets keyboard-up geometry with a stale keyboard-down state, so keyboardUp assertions and hide/refocus branches diverge from real runtime behavior.
Suggested fix
func setKeyboardHeightForTesting(_ height: CGFloat) {
stopKeyboardHeightAnimation()
- keyboardHeight = max(0, height)
+ let clampedHeight = max(0, height)
+ keyboardVisible = clampedHeight > 0
+ inputProxy.setKeyboardShown(keyboardVisible)
+ keyboardHeight = clampedHeight
+ updateDockedToolbarVisibility()
layoutRenderedTerminalForCurrentViewport()
layoutBottomDock()
syncSurfaceGeometry(shouldReassertNaturalSize: true)
}As per path instructions, correctness-critical detection should use a single authoritative source of truth and avoid disagreeing fallback state.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func setKeyboardHeightForTesting(_ height: CGFloat) { | |
| stopKeyboardHeightAnimation() | |
| keyboardHeight = max(0, height) | |
| layoutRenderedTerminalForCurrentViewport() | |
| layoutBottomDock() | |
| syncSurfaceGeometry(shouldReassertNaturalSize: true) | |
| func setKeyboardHeightForTesting(_ height: CGFloat) { | |
| stopKeyboardHeightAnimation() | |
| let clampedHeight = max(0, height) | |
| keyboardVisible = clampedHeight > 0 | |
| inputProxy.setKeyboardShown(keyboardVisible) | |
| keyboardHeight = clampedHeight | |
| updateDockedToolbarVisibility() | |
| layoutRenderedTerminalForCurrentViewport() | |
| layoutBottomDock() | |
| syncSurfaceGeometry(shouldReassertNaturalSize: true) | |
| } |
🤖 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/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift`
around lines 873 - 878, The test helper setKeyboardHeightForTesting(_:) is only
updating keyboardHeight, leaving the new keyboardVisible source of truth out of
sync with the injected state. Update this helper so it also derives and sets
keyboardVisible consistently from the passed height before calling
layoutRenderedTerminalForCurrentViewport(), layoutBottomDock(), and
syncSurfaceGeometry(shouldReassertNaturalSize:), matching the runtime behavior
used by keyboardUp checks and hide/refocus logic elsewhere in
GhosttySurfaceView.
Source: Path instructions
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.
Fixes the iOS chat keyboard scroll-edge bleed shown when the keyboard is active.\n\nChanged:\n- Adds regression coverage that exposes whether the iOS 26 top scroll-edge effect is forced while the chat keyboard viewport is clipped.\n- Leaves the transcript top edge on UIKit automatic during keyboard motion and while the keyboard is up, while keeping the bottom edge effect for the composer edge.\n\nVerified:\n- swift test --package-path Packages/iOS/CmuxAgentChatUI\n- xcodebuild test -workspace ios/cmux.xcworkspace -scheme cmux-ios -destination 'platform=iOS Simulator,id=C9D82663-886D-4EBD-92A0-96CF38A42FF3' -derivedDataPath /tmp/cmux-ios-scroll-edge -only-testing:cmuxUITests/cmuxUITests/testAgentChatTranscriptFrameMovesUpWithKeyboardAcrossScrollPositions\n- ./scripts/reload-cloud.sh --tag edge (fell back to local ./scripts/reload.sh --tag edge; succeeded)\n- ./ios/scripts/reload.sh --tag edge --simulator 'iPhone 17' (succeeded)\n\nDogfood:\n- macOS tag: http://127.0.0.1:17320/edge\n- iOS tag: edge\n- Physical iPhone reload was unavailable: cloud signing credentials missing, and device Aziz reported unavailable.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes the iOS chat scroll-edge “bleed” by moving chat to a UIKit keyboard host that tracks the keyboard frame and animation. The composer now stays pinned above the keyboard and the top overscroll is suppressed while typing on iOS 26.
Bug Fixes
ChatKeyboardTrackingViewControllerthat drives insets and offsets fromUIResponder.keyboardWillChangeFrame, keeping content stable during animations.Refactors
CmuxMobileSupport(keyboard transition/visibility, scroll viewport snapshot, shared composer controls) and adopted it inCmuxAgentChatUIandCmuxMobileTerminal.ChatTranscriptUITableViewfor deterministic insets; disabled nested SwiftUI keyboard avoidance inside chat.ChatAccessoryShortcutand a “Shared Shortcuts” settings scope reused from the terminal.CMUX_UITEST_AGENT_CHAT_PREVIEWandCMUX_UITEST_AGENT_CHAT_INLINE_PREVIEWfor UI testing.Written for commit 676631b. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests