Repository navigation
Fix mobile chat shortcut scroll edge blur - #7051
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthrough
Scroll Edge Fade Rework
Demo Chat Shortcuts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (23 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes the iOS chat composer shortcut row's edge fade by replacing a fixed alpha mask with a measurement-driven conditional gradient, ensuring the blur only activates when chips overflow the viewport and that chips at rest are not obscured.
Confidence Score: 5/5Safe to merge. Changes are scoped to a visual scroll-edge treatment in one SwiftUI chip row and a debug-only demo screen. The geometry measurement ordering is correct: onGeometryChange is applied before .padding in the modifier chain, so scrollContentWidth captures raw chip width and the comparison is accurate. The 34pt padding precisely matches the 34pt gradient width, so chip edges align with gradient boundaries at both scroll extents. The fade state is stable (onGeometryChange converges in two render passes with no oscillation). The AgentChatDemoScreen additions are entirely behind #if DEBUG and don't touch any production path. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[View renders\nscrollContentWidth=0\nscrollViewportWidth=0] --> B[onGeometryChange fires\nfor HStack content]
A --> C[onGeometryChange fires\nfor ScrollView viewport]
B --> D{scrollContentWidth >\nscrollViewportWidth + 1?}
C --> D
D -- No --> E[padding = 2pt\nmask = plain Rectangle]
D -- Yes --> F[padding = 34pt\nmask = leading + trailing\nLinearGradient curtains]
F --> G[At offset 0:\nfirst chip left-edge lands\nat gradient fully-opaque boundary\n→ chip fully visible]
F --> H[At max offset:\nlast chip right-edge aligns\nwith trailing gradient start\n→ chip fully visible]
E --> I[No fade artifacts\non short lists]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[View renders\nscrollContentWidth=0\nscrollViewportWidth=0] --> B[onGeometryChange fires\nfor HStack content]
A --> C[onGeometryChange fires\nfor ScrollView viewport]
B --> D{scrollContentWidth >\nscrollViewportWidth + 1?}
C --> D
D -- No --> E[padding = 2pt\nmask = plain Rectangle]
D -- Yes --> F[padding = 34pt\nmask = leading + trailing\nLinearGradient curtains]
F --> G[At offset 0:\nfirst chip left-edge lands\nat gradient fully-opaque boundary\n→ chip fully visible]
F --> H[At max offset:\nlast chip right-edge aligns\nwith trailing gradient start\n→ chip fully visible]
E --> I[No fade artifacts\non short lists]
Reviews (3): Last reviewed commit: "Keep shortcut chips outside edge fade" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| .overlay(alignment: .leading) { | ||
| if scrollNeedsEdgeBlur { | ||
| scrollEdgeBlur(isLeading: true) | ||
| } | ||
| } | ||
| .overlay(alignment: .trailing) { | ||
| if scrollNeedsEdgeBlur { | ||
| scrollEdgeBlur(isLeading: false) | ||
| } | ||
| } |
There was a problem hiding this comment.
Both curtains appear whenever scrollNeedsEdgeBlur is true, but at the initial scroll position (0) there is no content hidden behind the leading edge — the first chip is right at the left boundary. The 34pt leading curtain will overlap and visually obscure the first chip(s) for every overflowing row before the user has scrolled right at all, which is the common case. Only the trailing curtain is correct at position 0; the leading one should only appear once scrollOffset > 0. Tracking scroll position (e.g. via ScrollPosition on iOS 17+, or a preference-key offset reader) is needed to gate isLeading: true independently of isLeading: false.
| private var scrollNeedsEdgeBlur: Bool { | ||
| scrollContentWidth > scrollViewportWidth + 1 | ||
| } |
There was a problem hiding this comment.
scrollContentWidth measurement is taken on the view after .padding(.horizontal, 2) is applied, so it is always 4pt wider than the raw chip content. When compared against scrollViewportWidth with a tolerance of only + 1, this means the blur curtains can appear even when the chips fit the viewport exactly (4pt padding > 1pt tolerance). Subtracting the padding from the content measurement corrects the comparison.
| private var scrollNeedsEdgeBlur: Bool { | |
| scrollContentWidth > scrollViewportWidth + 1 | |
| } | |
| private var scrollNeedsEdgeBlur: Bool { | |
| // scrollContentWidth is measured on the padded HStack (2pt on each side = 4pt total), | |
| // so subtract the padding before comparing to the bare viewport width. | |
| scrollContentWidth - 4 > scrollViewportWidth + 1 | |
| } |
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!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/AgentChatDemoScreen.swift`:
- Around line 116-168: The preview accessory chips are exposing unsupported
actions as clickable no-ops. Update AgentChatDemoScreen’s
previewLeadingShortcuts and previewScrollableShortcuts/performPreviewShortcut so
only actions that actually work in this screen are rendered, or wire the
remaining TerminalInputAccessoryAction cases through the same input handling
path used by the real accessory; in particular, avoid generating chips whose
closure does nothing and ensure each shown ChatAccessoryShortcut maps to a
handled action.
🪄 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: fd7a4860-17a0-43d3-9ce4-b28bd782cf6c
📒 Files selected for processing (2)
Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Composer/ChatAccessoryChipRow.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/AgentChatDemoScreen.swift
| private var previewLeadingShortcuts: [ChatAccessoryShortcut] { | ||
| [ | ||
| ChatAccessoryShortcut( | ||
| id: "terminal.inputAccessory.hideKeyboard", | ||
| title: "", | ||
| systemImage: "keyboard.chevron.compact.down", | ||
| accessibilityLabel: L10n.string( | ||
| "terminal.input_accessory.hideKeyboard", | ||
| defaultValue: "Hide Keyboard" | ||
| ), | ||
| tint: .secondary, | ||
| semanticAction: .dismissKeyboard | ||
| ) {}, | ||
| ChatAccessoryShortcut( | ||
| id: "terminal.inputAccessory.composer", | ||
| title: "", | ||
| systemImage: "terminal", | ||
| accessibilityLabel: L10n.string( | ||
| "mobile.terminal.select", | ||
| defaultValue: "Terminal" | ||
| ) | ||
| ) {}, | ||
| ] | ||
| } | ||
|
|
||
| private func previewScrollableShortcuts(for stack: DemoStack) -> [ChatAccessoryShortcut] { | ||
| TerminalInputAccessoryAction.defaultConfigurableOrder.compactMap { action in | ||
| guard action.isSupportedInAgentChat else { return nil } | ||
| return ChatAccessoryShortcut( | ||
| id: action.accessibilityIdentifier, | ||
| title: action.title(isMacRemote: true), | ||
| systemImage: action.symbolName, | ||
| accessibilityLabel: action.accessibilityLabel ?? action.settingsDisplayName, | ||
| semanticAction: action == .paste ? .paste : nil | ||
| ) { | ||
| performPreviewShortcut(action, store: stack.store) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private func performPreviewShortcut( | ||
| _ action: TerminalInputAccessoryAction, | ||
| store: ChatConversationStore | ||
| ) { | ||
| switch action { | ||
| case .escape: | ||
| Task { await store.interrupt(hard: false) } | ||
| case .ctrlC: | ||
| Task { await store.interrupt(hard: true) } | ||
| default: | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Most of the new accessory chips are wired as no-ops.
previewLeadingShortcuts adds a Terminal chip with an empty action, and previewScrollableShortcuts builds chips for every agent-chat-supported terminal action even though performPreviewShortcut only handles .escape and .ctrlC. That means chips like tab/return/ctrlD/claude/codex/etc. are rendered as tappable but never do anything. Either limit this preview row to the actions this screen actually supports, or route the remaining actions through the same input path as the real accessory.
🤖 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 116 - 168, The preview accessory chips are exposing unsupported
actions as clickable no-ops. Update AgentChatDemoScreen’s
previewLeadingShortcuts and previewScrollableShortcuts/performPreviewShortcut so
only actions that actually work in this screen are rendered, or wire the
remaining TerminalInputAccessoryAction cases through the same input handling
path used by the real accessory; in particular, avoid generating chips whose
closure does nothing and ensure each shown ChatAccessoryShortcut maps to a
handled action.
…os-bottom-scroll-blur # Conflicts: # Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/AgentChatDemoScreen.swift
This branch was successfully deployed
1 participant
Summary
Fixes the iOS chat composer shortcut row edge treatment so horizontally scrolling glass chips blur out before they pass under pinned controls or the screen edge.
The old row used an alpha mask on the scroll view. With glass chips over glass chrome, that left a sharp glass-on-glass cutoff. The new row measures scroll content and viewport width, renders material blur curtains only when the row actually overflows, and keeps those curtains non-interactive so shortcuts remain tappable.
Validation
swift test --package-path Packages/iOS/CmuxAgentChatUI./scripts/reload-cloud.sh --tag blur(fleet unavailable locally, fell back to tagged local./scripts/reload.sh --tag blur)./ios/scripts/reload.sh --tag bluron iPhone 17 simulatordev.cmux.ios.blurwithCMUX_UITEST_AGENT_CHAT_INLINE_PREVIEW=1and captured/tmp/cmux-blur-proof/chat-preview-final.pngto verify the chat composer rendered and the non-overflow case no longer shows a dark edge blockNotes
The local Next.js helper server was started in a visible cmux helper terminal on port 9330.
/warmed with 200./handler/sign-inreturns 404 in this checkout without Stack auth secrets;/handler/after-sign-inreturns 307. Physical iPhone reload was unavailable becausedevicectllisted all paired devices as unavailable.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes the iOS chat shortcut row so horizontally scrolling chips fade at the edges only when the row overflows, and keeps chips outside the fade and tappable.
Bug Fixes
onGeometryChange.New Features
AgentChatDemoScreenusingCmuxMobileTerminalactions to preview scrolling and the edge fade.Written for commit f474e4a. Summary will update on new commits.
Summary by CodeRabbit