iOS: dismissable alt-screen sizing notice in terminal toolbar - #7669
Merged
Merged
Conversation
When the selected terminal session is in alt-screen mode (full-screen TUIs like Codex), the mirrored terminal renders at the Mac grid size and may not fill the phone frame. Show an orange (!) toolbar button whose popover explains this, with a persisted Don't Show Again action. The store already tracked per-surface active screen; this adds a public isAlternateScreen(surfaceID:) accessor in an extension and a standalone @observable AltScreenNoticeState (injected UserDefaults) so MobileShellComposite.swift itself has zero growth. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds alternate-screen notice support to the iOS mobile shell. It exposes alternate-screen state from terminal render-grid tracking, persists a user-controlled notice preference, adds a dismissible notice button, wires it into workspace and settings UI, and adds tests and localized strings. ChangesAlternate-screen notice feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 25✅ Passed checks (25 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9bc8102. Configure here.
Contributor
Greptile SummaryThis PR adds a dismissable orange (!) toolbar notice to the iOS workspace detail view when the selected terminal session is on the alternate screen (e.g. vim, Codex), explaining to users why the terminal may not fill the screen. The dismissal preference is persisted via
Confidence Score: 5/5This PR is safe to merge — it touches only UI state, toolbar layout, and a UserDefaults preference; no auth, sync, or terminal rendering paths are changed in a breaking way. The change is narrowly scoped: the same-value guard on terminalActiveScreenBySurfaceID prevents over-notification without altering delivery semantics, isAlternateScreen is a pure dictionary read, AltScreenNoticeButton holds no store reference, and showAltScreenNotice follows the established MobileDisplaySettings pattern with correct tri-state default handling. Tests cover alt-screen tracking, observation coalescing, and settings persistence. No blocking issues found. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[render-grid frame arrives] --> B{same activeScreen as tracked?}
B -- yes --> C[skip write - no Observable notification]
B -- no --> D[write terminalActiveScreenBySurfaceID]
D --> E[Observable notifies WorkspaceDetailView]
E --> F{selectedTerminalID + isAlternateScreen + showAltScreenNotice?}
F -- all true --> G[emit workspace-altscreen-notice ToolbarItem]
G --> H[AltScreenNoticeButton shown]
H --> I{user taps button}
I --> J[popover appears]
J --> K{user taps Don't Show Again}
K --> L[showAltScreenNotice = false persisted to UserDefaults]
L --> M[SwiftUI re-render: popover dismissed, ToolbarItem removed]
F -- any false --> N[no ToolbarItem]
Q[Settings Terminal toggle] -.-> L
%%{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[render-grid frame arrives] --> B{same activeScreen as tracked?}
B -- yes --> C[skip write - no Observable notification]
B -- no --> D[write terminalActiveScreenBySurfaceID]
D --> E[Observable notifies WorkspaceDetailView]
E --> F{selectedTerminalID + isAlternateScreen + showAltScreenNotice?}
F -- all true --> G[emit workspace-altscreen-notice ToolbarItem]
G --> H[AltScreenNoticeButton shown]
H --> I{user taps button}
I --> J[popover appears]
J --> K{user taps Don't Show Again}
K --> L[showAltScreenNotice = false persisted to UserDefaults]
L --> M[SwiftUI re-render: popover dismissed, ToolbarItem removed]
F -- any false --> N[no ToolbarItem]
Q[Settings Terminal toggle] -.-> L
Reviews (7): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @MainActor | ||
| @Observable | ||
| public final class AltScreenNoticeState { | ||
| static let dismissedDefaultsKey = "mobile.altScreenNotice.dismissed" |
Contributor
There was a problem hiding this comment.
internal on this public class, exposing the UserDefaults key string to the rest of the module without a clear reason. Marking it private closes that surface.
Suggested change
| static let dismissedDefaultsKey = "mobile.altScreenNotice.dismissed" | |
| private static let dismissedDefaultsKey = "mobile.altScreenNotice.dismissed" |
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!
Cursor review: per-view @State instances meant a detail view opened before Don't Show Again could still show the notice. All views now read the same shared instance, so dismissal hides the button everywhere immediately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
![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/CmuxMobileShell/Sources/CmuxMobileShell/AltScreenNoticeState.swift`:
- Around line 8-9: Remove the new singleton from AltScreenNoticeState and rely
on scoped injection instead. AltScreenNoticeState already has init(defaults:)
for dependency injection, so delete the static shared instance and pass an
AltScreenNoticeState from the parent coordinator or environment into
WorkspaceDetailView. Update any call sites that currently reference
AltScreenNoticeState.shared to use the injected instance, keeping the dismissal
state owned by the scoped type rather than global app state.
🪄 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: a8d69ac8-4733-4aa8-838b-4c85ff12076b
📒 Files selected for processing (2)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/AltScreenNoticeState.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift
package-conventions-lint bans singletons; the shared instance now lives as @State on WorkspaceShellView and flows through WorkspaceDetailContainer to each WorkspaceDetailView, keeping one dismissal source of truth per navigation tree without a static. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Don't Show Again had no way back. The persisted flag moves into MobileDisplaySettings as showAltScreenNotice (default on, didSet write-through like the other preferences), read via the environment; Settings > Terminal gains a localized toggle bound to it. Deletes the standalone AltScreenNoticeState and its root-to-detail plumbing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
recordTerminalRenderGridDelivery wrote the observation-tracked terminalActiveScreenBySurfaceID dict on every render-grid frame; @observable fires on same-value assignments, so the workspace toolbar (which reads isAlternateScreen) re-evaluated at terminal frame rate and rebuilds racing the popover-dismiss animation flickered the (!) item. All tracked-screen writes are now change-conditional, with an observation-tracking regression test proving same-value frames no longer notify. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Multi-line Texts in a popover report a single-line ideal height, so the compact-width popover truncated the warning. fixedSize(horizontal: false, vertical: true) on the title and explanation makes them report their full wrapped height, and the popover sizes to fit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
When the selected terminal session is in alt-screen mode (full-screen TUIs like Codex, or Claude Code with alt-screen enabled), the phone mirrors the Mac grid and letterboxes instead of filling the frame. Users read this as a rendering bug. This adds an orange (!) button to the workspace terminal toolbar whenever the active surface is on the alternate screen; tapping it shows a popover explaining why the terminal may not fill the screen, with a Don't Show Again action persisted across launches.
The store already tracked per-surface active screen (
terminalActiveScreenBySurfaceID, fed by every render-grid frame). This PR adds a publicisAlternateScreen(surfaceID:)accessor in an extension (MobileShellComposite+AltScreenNotice.swift) and a standalone@MainActor @Observable AltScreenNoticeStatewith an injectedUserDefaultsfor the dismissal flag, soMobileShellComposite.swiftitself has zero line growth.WorkspaceDetailViewconditionally emits aToolbarItem(workspace-altscreen-notice) before the trailing cluster; the button and popover live inAltScreenNoticeButton.swift(value inputs + action closure only, no store reference).Strings are
mobile.altScreenNotice.*inios/cmux/Resources/Localizable.xcstringswith English and Japanese entries, read viaL10n.string.Tests (
MobileShellAltScreenNoticeTests): render-grid frames driveisAlternateScreenthrough the real delivery seam (.alternatetrue,.primaryfalse, unknown surface false), and dismissal persists acrossAltScreenNoticeStateinstances on a scopedUserDefaultssuite.Verified on tag
altwrn: simulator paired to the tagged Mac app, vim in the mirrored surface -> (!) appears in the toolbar; quitting alt-screen hides it. Package tests pass on host.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Low Risk
UI and display preferences only; terminal delivery change is a no-op when screen mode is unchanged, with regression tests for observation.
Overview
Adds an orange toolbar warning when the selected terminal is in alternate (full-screen TUI) mode, so users understand letterboxing instead of treating it as a rendering bug.
MobileShellCompositeexposesisAlternateScreen(surfaceID:)for the UI. Render-grid delivery now skips same-value writes toterminalActiveScreenBySurfaceIDso SwiftUI observation does not re-fire on every frame (toolbar flicker fix).AltScreenNoticeButtonshows a popover explaining Mac-sized mirroring, with Don't Show Again wired toMobileDisplaySettings.showAltScreenNotice(UserDefaults, default on).WorkspaceDetailViewinserts the toolbar item when alt-screen is active and the preference is on; Settings > Terminal adds a toggle to re-enable the notice. English and Japanese strings added.Tests cover alt-screen tracking, observation behavior, and settings persistence.
Reviewed by Cursor Bugbot for commit 58e7e4b. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds a dismissable orange (!) notice in the iOS terminal toolbar when the selected session is on the alternate screen, explaining why full‑screen TUIs may not fill the phone. You can hide it with “Don’t Show Again” or re‑enable it in Settings; the popover now fits long text and the toolbar no longer flickers on unchanged frames.
New Features
isAlternateScreen(surfaceID:)is true andMobileDisplaySettings.showAltScreenNoticeis on; the popover explains mirroring and offers “Don’t Show Again” persisted inUserDefaults.WorkspaceDetailViewreadsMobileDisplaySettingsfrom the environment; localized EN/JA strings and tests cover detection and preference persistence.Bug Fixes
terminalActiveScreenBySurfaceIDto prevent toolbar flicker; regression test confirms unchanged frames don’t notify observers.Written for commit 58e7e4b. Summary will update on new commits.
Summary by CodeRabbit