Repository navigation
Fix iOS chat toolbar glass separation - #7111
azooz2003-bit wants to merge 8 commits into
Conversation
…os-bottom-scroll-blur # Conflicts: # Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/AgentChatDemoScreen.swift
…-blur # Conflicts: # Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/AgentChatDemoScreen.swift # Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceChatPane.swift
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds an isolated glass toolbar control, applies it to workspace and demo back buttons, forces the demo descriptor into idle from an environment variable, removes an iOS-only composer glass wrapper, and switches accessory chip fading to the trailing side. ChangesIsolated glass toolbar control and demo state override
Accessory chip trailing fade
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 toolbar glass separation by keeping the workspace title in the principal slot, wrapping back-button controls in a new
Confidence Score: 4/5The toolbar glass separation and demo screen changes are safe; the chip row refactor has two correctness issues that should be addressed before shipping. The glass-separation fix in the toolbar (new helper, removed GlassEffectContainer, back-button callsites) is clean and well-verified. Two issues are present in the reworked ChatAccessoryChipRow: the trailing-fade threshold omits the always-present 14 pt leading padding, leaving a ~16 pt range where the row is scrollable but shows no visual indicator; and the hard-stop overlay uses hardcoded Color.black, which renders as a visible black rectangle on light-mode or non-dark glass backgrounds. Both are introduced by this PR in production code paths. Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Composer/ChatAccessoryChipRow.swift — both issues are here Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[ChatAccessoryChipRow body] --> B{displayedLeadingShortcuts not empty?}
B -- yes --> C[leadingShortcutCluster layoutPriority 2 zIndex 1]
B -- no --> D{displayedScrollableShortcuts not empty?}
C --> D
D -- yes --> E[scrollableShortcutStrip]
D -- no --> Z[Empty height 32]
E --> F[ScrollView .horizontal]
F --> G[chips HStack onGeometryChange scrollContentWidth]
G --> H[.padding leading 14pt always .padding trailing 34 if fade else 2]
H --> I[.compositingGroup .clipShape rect .mask scrollTrailingFadeMask]
I --> J[.overlay leading scrollLeadingHardStop Color.black 14pt]
I --> K{scrollNeedsTrailingFade? scrollContentWidth > scrollViewportWidth + 1}
K -- true --> L[trailing fade gradient + 34pt trailing padding]
K -- false --> M[full opaque mask + 2pt trailing padding]
L --> N[Fade shown]
M --> O{actual content scrollContentWidth + 14 + 2 > scrollViewportWidth?}
O -- yes 16pt dead zone --> P[Scrollable but NO fade shown]
O -- no --> Q[No scroll needed]
%%{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[ChatAccessoryChipRow body] --> B{displayedLeadingShortcuts not empty?}
B -- yes --> C[leadingShortcutCluster layoutPriority 2 zIndex 1]
B -- no --> D{displayedScrollableShortcuts not empty?}
C --> D
D -- yes --> E[scrollableShortcutStrip]
D -- no --> Z[Empty height 32]
E --> F[ScrollView .horizontal]
F --> G[chips HStack onGeometryChange scrollContentWidth]
G --> H[.padding leading 14pt always .padding trailing 34 if fade else 2]
H --> I[.compositingGroup .clipShape rect .mask scrollTrailingFadeMask]
I --> J[.overlay leading scrollLeadingHardStop Color.black 14pt]
I --> K{scrollNeedsTrailingFade? scrollContentWidth > scrollViewportWidth + 1}
K -- true --> L[trailing fade gradient + 34pt trailing padding]
K -- false --> M[full opaque mask + 2pt trailing padding]
L --> N[Fade shown]
M --> O{actual content scrollContentWidth + 14 + 2 > scrollViewportWidth?}
O -- yes 16pt dead zone --> P[Scrollable but NO fade shown]
O -- no --> Q[No scroll needed]
Reviews (3): Last reviewed commit: "Isolate iOS composer shortcut scroll" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private func demoDescriptor(from descriptor: ChatSessionDescriptor) -> ChatSessionDescriptor { | ||
| let demoState = ProcessInfo.processInfo.environment["CMUX_UITEST_AGENT_CHAT_STATE"]? | ||
| .trimmingCharacters(in: .whitespacesAndNewlines) | ||
| guard demoState == "idle" else { | ||
| return descriptor | ||
| } | ||
| return descriptor.withState(.idle) | ||
| } |
There was a problem hiding this comment.
Sources/
demoDescriptor(from:) reads the CMUX_UITEST_AGENT_CHAT_STATE environment variable — a named UI-test integration point — and lives under Packages/iOS/CmuxMobileShellUI/Sources/, a production Sources/ path. The no-test-debug-seam-in-production-source rule flags debug-only seams in **/Sources/** regardless of a surrounding #if DEBUG guard, and the pass condition for #if DEBUG blocks is "real product behavior" — screenshots and simulator fixture overrides are test behavior, not product behavior. The conventional fix is to place this configuration in a dedicated debug module, a UI-test support target, or a test-only launch-argument resolver that is never compiled into the shipping product directory.
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!
| /// Compact toolbar glass that must remain visually separate from adjacent | ||
| /// glass controls. | ||
| @ViewBuilder | ||
| func mobileIsolatedGlassCompactToolbarControl(horizontalPadding: CGFloat = 14) -> some View { |
There was a problem hiding this comment.
horizontalPadding value of 14 is never exercised in practice — both call sites in WorkspaceChatPane and AgentChatDemoScreen explicitly pass 16. An unused default creates ambiguity about which value is correct for the back-button context and makes the API misleading for future callers. If 16 is the intended standard for toolbar back controls, the default should match; if the default is reserved for a different, narrower use case, a comment explaining that distinction would prevent accidental misuse.
| func mobileIsolatedGlassCompactToolbarControl(horizontalPadding: CGFloat = 14) -> some View { | |
| func mobileIsolatedGlassCompactToolbarControl(horizontalPadding: CGFloat = 16) -> some View { |
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 127-134: The demo-state override in demoDescriptor(from:) is
reading the environment directly instead of using the shared
UITestConfig.value(for:env:) helper, which is inconsistent with
inlineWorkspaceTitle and inlineWorkspaceSubtitle. Update demoDescriptor(from:)
to fetch CMUX_UITEST_AGENT_CHAT_STATE through UITestConfig.value(for:env:), then
keep the existing trim and idle-state check so the demo-state behavior matches
the rest of AgentChatDemoScreen.
🪄 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: ff5e635f-23b5-49a3-b1d1-fd209023e46e
📒 Files selected for processing (4)
Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Composer/ChatAccessoryChipRow.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/AgentChatDemoScreen.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceChatPane.swiftPackages/iOS/CmuxMobileSupport/Sources/CmuxMobileSupport/View+MobileGlass.swift
| private func demoDescriptor(from descriptor: ChatSessionDescriptor) -> ChatSessionDescriptor { | ||
| let demoState = ProcessInfo.processInfo.environment["CMUX_UITEST_AGENT_CHAT_STATE"]? | ||
| .trimmingCharacters(in: .whitespacesAndNewlines) | ||
| guard demoState == "idle" else { | ||
| return descriptor | ||
| } | ||
| return descriptor.withState(.idle) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm UITestConfig.value's trimming/matching semantics before reuse.
rg -n -A 15 'struct UITestConfig|enum UITestConfig' --type=swift
rg -n -A 15 'func value\(' --type=swift -g '*UITestConfig*'Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- AgentChatDemoScreen.swift slice ---'
sed -n '1,220p' Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/AgentChatDemoScreen.swift
echo
echo '--- Search for UITestConfig and value(for:) ---'
rg -n 'UITestConfig|value\(for:|CMUX_UITEST_AGENT_CHAT_STATE|inlineWorkspaceTitle|inlineWorkspaceSubtitle' Packages -g '*.swift'Repository: manaflow-ai/cmux
Length of output: 34268
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- UITestConfig.swift ---'
sed -n '1,260p' Packages/iOS/CmuxMobileSupport/Sources/CmuxMobileSupport/UITestConfig.swift
echo
echo '--- UITestConfigTests.swift ---'
sed -n '1,220p' Packages/iOS/CmuxMobileSupport/Tests/CmuxMobileSupportTests/UITestConfigTests.swiftRepository: manaflow-ai/cmux
Length of output: 13404
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' Packages/iOS/CmuxMobileSupport/Sources/CmuxMobileSupport/UITestConfig.swiftRepository: manaflow-ai/cmux
Length of output: 7795
Use UITestConfig.value(for:env:) here too.
inlineWorkspaceTitle and inlineWorkspaceSubtitle already route UI-test overrides through the shared helper, which handles the trim + mock-data gating consistently. This keeps the demo-state override aligned with the rest of the file.
🤖 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 127 - 134, The demo-state override in demoDescriptor(from:) is
reading the environment directly instead of using the shared
UITestConfig.value(for:env:) helper, which is inconsistent with
inlineWorkspaceTitle and inlineWorkspaceSubtitle. Update demoDescriptor(from:)
to fetch CMUX_UITEST_AGENT_CHAT_STATE through UITestConfig.value(for:env:), then
keep the existing trim and idle-state check so the demo-state behavior matches
the rest of AgentChatDemoScreen.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Composer/ChatAccessoryChipRow.swift">
<violation number="1" location="Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Composer/ChatAccessoryChipRow.swift:197">
P2: The leading hard-stop overlay is painted black, not used as a mask, so it can render a visible black bar in the chip row. Use a background-matching style/material or remove the visible fill instead of hard-coding black.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| private var scrollLeadingHardStop: some View { | ||
| Rectangle() | ||
| .fill(Color.black) |
There was a problem hiding this comment.
P2: The leading hard-stop overlay is painted black, not used as a mask, so it can render a visible black bar in the chip row. Use a background-matching style/material or remove the visible fill instead of hard-coding black.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Composer/ChatAccessoryChipRow.swift, line 197:
<comment>The leading hard-stop overlay is painted black, not used as a mask, so it can render a visible black bar in the chip row. Use a background-matching style/material or remove the visible fill instead of hard-coding black.</comment>
<file context>
@@ -170,17 +192,22 @@ public struct ChatAccessoryChipRow: View {
+ private var scrollLeadingHardStop: some View {
+ Rectangle()
+ .fill(Color.black)
+ .frame(width: Self.scrollLeadingHardStopWidth)
+ .overlay(alignment: .trailing) {
</file context>
|
Superseded by #7116 so the iOS toolbar fixes can be tested and reviewed in one PR. |
This branch was successfully deployed
1 participant
Follow-up to #7051.
Fixes the iOS inline agent chat toolbar after simulator verification showed the earlier build still fused the back button and title glass.
Changes:
Verification:
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Separated the iOS chat toolbar glass so the back button and title render independently. Fixed the composer shortcut row by isolating the scroll strip with a leading hard stop and a trailing blur, keeping chips fully visible.
Bug Fixes
mobileIsolatedGlassCompactToolbarControl(...)inCmuxMobileSupportand applied to back controls inCmuxMobileShellUI(WorkspaceChatPane,AgentChatDemoScreen) with 16pt horizontal padding; title stays in the principal slot so glass remains separated.ChatAccessoryChipRowinCmuxAgentChatUI: split a fixed leading cluster from a scrollable strip, added a leading hard stop and trailing-only fade with clipping/padding to keep chips outside the fade and correct scrolling; removed the composer glass wrapper inChatComposerViewto avoid unintended blur.New Features
CMUX_UITEST_AGENT_CHAT_STATE=idleto force idle state for simulator screenshots.Written for commit 8d523c7. Summary will update on new commits.
Summary by CodeRabbit
New Features
UI Improvements