Add global UI scale setting and zoom shortcuts (#3862) - #3864
Closed
austinywang wants to merge 48 commits into
Closed
austinywang wants to merge 48 commits into
austinywang wants to merge 48 commits into
Conversation
Add regression coverage for the requested UI scale behavior before wiring the production path. The tests cover cmux.json round-trip persistence, root environment propagation into a representative sidebar view, and shortcut-driven clamp/reset behavior. Constraint: Test-first policy for issue 3862 requires this commit to fail before the fix commit lands Confidence: high Scope-risk: narrow Tested: Not run locally per repository policy Not-tested: CI execution pending after implementation
Issue 3862 needs UI zoom to be independent from terminal font size and browser zoom. This adds app.uiScale as the single persisted setting, injects a clamped environment value from the app roots, and routes the settings slider plus customizable Cmd-Shift zoom shortcuts through the same persistence path. The scale is applied to the sidebar chrome/workspace rows, right sidebar mode bar, file browser AppKit rows/search/header, settings chrome, markdown panel, and task manager so built-in UI surfaces follow one proportional value instead of new per-view constants. Constraint: Do not change Ghostty font-size or browser zoom shortcuts Constraint: Local tests are not run by policy; CI owns test execution Rejected: Per-surface font-size keys for v1 | would duplicate state before the global scale source of truth exists Confidence: medium Scope-risk: moderate Tested: Static diff check and JSON validation with jq Not-tested: Local XCTest/build per repository and user policy
|
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:
📝 WalkthroughWalkthroughAdds a persisted UI scale setting with UIScaleSettings, three UI-zoom shortcuts and AppDelegate interception, settings-file persistence and schema, environment injection of uiScaleFactor, scaled font helpers, broad view updates to honor scaling, tests, and project wiring. ChangesUI Scale Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (11 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 |
CI failed before compiling the new UI scale implementation because the new app source and regression test were present on disk but absent from the Xcode project target membership. Add both files to the app/test source phases so the CI build exercises the intended code path. Constraint: The requested verification path forbids local tests and direct xcodebuild, so this fix is validated with project-file linting and CI.\nConfidence: high\nScope-risk: narrow\nTested: git diff --check; plutil -lint GhosttyTabs.xcodeproj/project.pbxproj\nNot-tested: Local build or unit tests, per instruction
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Sources/ContentView.swift (1)
13484-13497:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winScale all PR status icon variants, not just
.closed.
PullRequestStatusIconappliesuiScaleFactoronly for the.closedsymbol path..openand.mergedstay fixed-size, so icon sizing becomes inconsistent when UI scale changes.Suggested fix
private struct PullRequestStatusIcon: View { `@Environment`(\.uiScaleFactor) private var uiScaleFactor let status: SidebarPullRequestStatus let color: Color private static let frameSize: CGFloat = 12 var body: some View { switch status { case .open: - PullRequestOpenIcon(color: color) + PullRequestOpenIcon(color: color) + .scaleEffect(uiScaleFactor) case .merged: - PullRequestMergedIcon(color: color) + PullRequestMergedIcon(color: color) + .scaleEffect(uiScaleFactor) case .closed: Image(systemName: "xmark.circle") .font(.system(size: UIScaleSettings.scaled(7, by: uiScaleFactor), weight: .regular)) .foregroundColor(color) .frame( width: UIScaleSettings.scaled(Self.frameSize, by: uiScaleFactor), height: UIScaleSettings.scaled(Self.frameSize, by: uiScaleFactor) ) } } }Also applies to: 13501-13537, 13539-13573
🤖 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 `@Sources/ContentView.swift` around lines 13484 - 13497, PullRequestStatusIcon currently only scales the `.closed` branch; update the `.open` and `.merged` branches so their icons respect uiScaleFactor too by applying the same sizing logic used for the `.closed` case. Locate the switch in ContentView.swift (the PullRequestStatusIcon switch with cases `.open`, `.merged`, `.closed`) and either make PullRequestOpenIcon and PullRequestMergedIcon accept uiScaleFactor and Self.frameSize or wrap them with the same .font, .foregroundColor, and .frame calls that use UIScaleSettings.scaled(Self.frameSize, by: uiScaleFactor) so all three variants are consistently sized; apply the same change to the other occurrences noted (the ranges 13501-13537 and 13539-13573).Sources/SessionIndexView.swift (2)
2202-2204:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPopoverRow equality should include
uiScaleFactorto prevent stale rendering on scale changes
PopoverRowusesuiScaleFactorin its body for font scaling but only comparesentryin its==implementation. If the view is rendered with.equatable(), scale changes won't trigger re-renders.Proposed fix
static func == (lhs: PopoverRow, rhs: PopoverRow) -> Bool { - lhs.entry == rhs.entry + lhs.entry == rhs.entry && + lhs.uiScaleFactor == rhs.uiScaleFactor }🤖 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 `@Sources/SessionIndexView.swift` around lines 2202 - 2204, The Equatable implementation for PopoverRow only compares lhs.entry to rhs.entry, which ignores uiScaleFactor and can cause stale views when scale changes; update the static func ==(lhs: PopoverRow, rhs: PopoverRow) to also compare lhs.uiScaleFactor == rhs.uiScaleFactor (in addition to entry) so changes in uiScaleFactor will trigger re-renders when using .equatable().
1659-1681:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUI scale is not bridged into popover-hosted SwiftUI roots
Both
SessionTranscriptPopoverHostandSectionPopoverHostrebuild content inside manualNSHostingControllerroots without injectinguiScaleFactorfrom the parent environment. Popovers will render with default scale even when the parent sidebar scales.The fix requires three steps for each host:
- Add
@Environment(\.uiScaleFactor)to the host struct- Pass
uiScaleFactorthrough the coordinator'supdate()method- Apply
.environment(\.uiScaleFactor, currentUIScaleFactor)to the hosted root view before assigning tohostingController.rootViewProposed fix
private struct SessionTranscriptPopoverHost: NSViewRepresentable { + `@Environment`(\.uiScaleFactor) private var uiScaleFactor `@Binding` var isPresented: Bool let entry: SessionEntry @@ func updateNSView(_ nsView: PopoverAnchorView, context: Context) { let coordinator = context.coordinator coordinator.anchorView = nsView - coordinator.update(entry: entry) + coordinator.update(entry: entry, uiScaleFactor: uiScaleFactor) if isPresented { coordinator.present() } else { coordinator.dismiss() } @@ final class Coordinator: NSObject, NSPopoverDelegate { @@ + private var currentUIScaleFactor: Double = 1.0 @@ - func update(entry: SessionEntry) { + func update(entry: SessionEntry, uiScaleFactor: Double) { let shouldRefresh = currentEntry?.id != entry.id currentEntry = entry + currentUIScaleFactor = uiScaleFactor if shouldRefresh { refreshContent() } } @@ hostingController.rootView = AnyView( SessionTranscriptPreviewView( entry: entry, sizeModel: sizeModel, onResize: { [weak self] proposedSize in self?.resize(to: proposedSize) } ) { [weak self] in self?.closeFromContent() } .id(entry.id) + .environment(\.uiScaleFactor, currentUIScaleFactor) ) } } } struct SectionPopoverHost: NSViewRepresentable { + `@Environment`(\.uiScaleFactor) private var uiScaleFactor `@Binding` var isPresented: Bool @@ func updateNSView(_ nsView: NSView, context: Context) { let coordinator = context.coordinator coordinator.anchorView = nsView coordinator.update( section: section, search: search, loadSnapshot: loadSnapshot, - onResume: onResume + onResume: onResume, + uiScaleFactor: uiScaleFactor ) if isPresented { coordinator.present() } else { coordinator.dismiss() } } @@ final class Coordinator: NSObject, NSPopoverDelegate { @@ + private var currentUIScaleFactor: Double = 1.0 @@ func update( section: IndexSection, search: `@escaping` SessionSearchFn, loadSnapshot: `@escaping` DirectorySnapshotFn, - onResume: ((SessionEntry) -> Void)? + onResume: ((SessionEntry) -> Void)?, + uiScaleFactor: Double ) { currentSection = section currentSearch = search currentLoadSnapshot = loadSnapshot currentOnResume = onResume + currentUIScaleFactor = uiScaleFactor guard popover?.isShown == true else { return } guard lastRenderedSection != section || lastRenderedPresentationCount != presentationCount else { return } refreshContent() } @@ hostingController.rootView = AnyView( SectionPopoverView( section: section, search: search, loadSnapshot: loadSnapshot, onResume: onResume ) { [weak self] in self?.closeFromContent() } .id(identity) + .environment(\.uiScaleFactor, currentUIScaleFactor) ) } } }🤖 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 `@Sources/SessionIndexView.swift` around lines 1659 - 1681, SessionTranscriptPopoverHost and SectionPopoverHost create NSHostingController roots without inheriting the parent uiScaleFactor, so add `@Environment`(\\.uiScaleFactor) private var uiScaleFactor to each host struct, update the Coordinator.update(...) signatures to accept the current uiScaleFactor (e.g., update(entry:..., uiScaleFactor: Double) or similar), pass the host's uiScaleFactor into coordinator.update(...) from updateNSView, and before assigning hostingController.rootView set the hosted SwiftUI view with .environment(\\.uiScaleFactor, uiScaleFactor) so the popover-hosted root receives the same scale as the parent.
🤖 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 `@Sources/AppDelegate.swift`:
- Around line 10729-10731: The UI-scale shortcut handler
(handleUIScaleShortcut(event:)) is being invoked even when the command palette
is active; update the call sites so they first check the command palette
visibility and skip UI-scale handling while it's shown. Concretely, before
calling handleUIScaleShortcut(event:), add a guard like if
commandPaletteIsVisible() { return false } (or use the existing command palette
visibility API such as commandPaletteController?.isVisible) so the function is
not invoked when the palette is open; apply this same guard at each place that
currently calls handleUIScaleShortcut (the earlier UI-scale routing locations
mentioned) to ensure zoom shortcuts are blocked while the palette is active.
In `@Sources/SettingsNavigation.swift`:
- Line 295: The new UI Scale setting was added to the search index but not to
the navigation anchors, so add "app.uiScale" to the settingsPathAnchorIDs array
to keep path-based routing consistent; update the settingsPathAnchorIDs constant
(the collection that contains anchor IDs used for jump-to navigation) to include
"app.uiScale" alongside the other "app.*" entries so the settingsPath anchor
routing/highlighting resolves for the new setting.
In `@Sources/UIScaleSettings.swift`:
- Line 38: Replace the runtime NSLog call in UIScaleSettings with Swift's
unified Logger: add a Logger instance (e.g., a static let logger on
UIScaleSettings or a module-level Logger) and change the NSLog invocation that
references jsonPath and error to a structured logger call (e.g., logger.error or
logger.log) including both jsonPath and String(describing: error) in the
message; ensure OSLog/Logger is imported and use a clear subsystem/category for
the logger to match project conventions.
---
Outside diff comments:
In `@Sources/ContentView.swift`:
- Around line 13484-13497: PullRequestStatusIcon currently only scales the
`.closed` branch; update the `.open` and `.merged` branches so their icons
respect uiScaleFactor too by applying the same sizing logic used for the
`.closed` case. Locate the switch in ContentView.swift (the
PullRequestStatusIcon switch with cases `.open`, `.merged`, `.closed`) and
either make PullRequestOpenIcon and PullRequestMergedIcon accept uiScaleFactor
and Self.frameSize or wrap them with the same .font, .foregroundColor, and
.frame calls that use UIScaleSettings.scaled(Self.frameSize, by: uiScaleFactor)
so all three variants are consistently sized; apply the same change to the other
occurrences noted (the ranges 13501-13537 and 13539-13573).
In `@Sources/SessionIndexView.swift`:
- Around line 2202-2204: The Equatable implementation for PopoverRow only
compares lhs.entry to rhs.entry, which ignores uiScaleFactor and can cause stale
views when scale changes; update the static func ==(lhs: PopoverRow, rhs:
PopoverRow) to also compare lhs.uiScaleFactor == rhs.uiScaleFactor (in addition
to entry) so changes in uiScaleFactor will trigger re-renders when using
.equatable().
- Around line 1659-1681: SessionTranscriptPopoverHost and SectionPopoverHost
create NSHostingController roots without inheriting the parent uiScaleFactor, so
add `@Environment`(\\.uiScaleFactor) private var uiScaleFactor to each host
struct, update the Coordinator.update(...) signatures to accept the current
uiScaleFactor (e.g., update(entry:..., uiScaleFactor: Double) or similar), pass
the host's uiScaleFactor into coordinator.update(...) from updateNSView, and
before assigning hostingController.rootView set the hosted SwiftUI view with
.environment(\\.uiScaleFactor, uiScaleFactor) so the popover-hosted root
receives the same scale as the parent.
🪄 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: fbda58aa-2fec-41ff-88ba-4203fe5580dc
📒 Files selected for processing (23)
Resources/Localizable.xcstringsSources/AppDelegate.swiftSources/CmuxSettingsJSONPathSupport.swiftSources/ContentView.swiftSources/FileExplorerStore.swiftSources/FileExplorerView.swiftSources/KeyboardShortcutSettings.swiftSources/KeyboardShortcutSettingsFileStore+Template.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/Panels/MarkdownPanelView.swiftSources/RightSidebarPanelView.swiftSources/SessionIndexView.swiftSources/Settings/ConfigSettingsView.swiftSources/SettingsNavigation.swiftSources/SettingsSearchAliases.swiftSources/ShortcutHintPill.swiftSources/TaskManagerView.swiftSources/TaskManagerWindowController.swiftSources/UIScaleSettings.swiftSources/cmuxApp.swiftcmuxTests/UIScaleSettingsTests.swiftweb/data/cmux-shortcuts.tsweb/data/cmux.schema.json
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Review feedback exposed two risks in the first pass: shortcut-triggered scale changes performed settings-file writes on the key event path, and several manually hosted or equatable views could miss the propagated scale. Move cmux.json persistence behind a serial background queue, keep shortcut routing out of command-palette state, and thread the scale into popover and Task Manager roots through SwiftUI state instead of an observer. Constraint: Direct local tests and xcodebuild are prohibited for this task; validation must come from static checks and CI.\nRejected: Leave persistence synchronous for test determinism | it violates the keyboard event hot-path constraint.\nConfidence: high\nScope-risk: narrow\nTested: git diff --check; plutil -lint GhosttyTabs.xcodeproj/project.pbxproj; jq empty Resources/Localizable.xcstrings web/data/cmux.schema.json web/data/cmux-settings.schema.json\nNot-tested: Local build or unit tests, per instruction
Contributor
Greptile SummaryThis PR adds a global
Confidence Score: 5/5Safe to merge; the implementation is thorough and the many regressions caught in earlier rounds have all been addressed. The two remaining findings are both in ConfigSettingsView: a status-message label and a banner label that lost Dynamic Type adaptation when their fonts were switched to fixed-size cmuxFont calls. These do not affect the correctness of the scale feature or cause data loss, and the one-line fix (adding relativeTo:) is clear. All other previously-flagged issues — off-main I/O, generation-guarded reloads, file-explorer expansion restoration, header-height constraint, popover visibility guards, and locale coverage — are confirmed fixed in the current code. Sources/Settings/ConfigSettingsView.swift — two font calls need relativeTo: to restore Dynamic Type adaptation. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["User action — slider or keyboard shortcut"] --> B["UIScaleSettings.set() on MainActor"]
B --> C["UserDefaults update"]
B --> D["recordPendingPersistence via OSAllocatedUnfairLock"]
B --> E["NotificationCenter post uiScaleDidChange"]
B --> F["Fire Task to persistence coordinator"]
F --> G["UIScaleSettingsPersistenceCoordinator actor\n150 ms debounce via Task.sleep"]
G --> H["writeAppUIScaleOffMain\nwithCheckedThrowingContinuation + named DispatchQueue"]
H --> I["CmuxSettingsJSONCEditor\nJSONC-preserving patch of settings file"]
H --> J["MainActor: completePendingPersistence → settingsFileStore.reload()"]
C --> K["AppStorage trigger — MainWindowUIScaleRoot / TaskManagerRootView"]
K --> L["environment uiScaleFactor injected into SwiftUI tree"]
L --> M["SwiftUI chrome — sidebars, settings, session index, markdown"]
L --> N["AppKit surfaces — updateUIScale, reloadData, restoreExpansionState"]
Reviews (30): Last reviewed commit: "fix: pass ui scale in session popover te..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Follow-up review found that reset-all could update only AppStorage while leaving cmux.json stale, and that PR status icons still had unscaled drawing internals. Persist reset-all through the same UI scale path, debounce background writes from live slider updates, make shortcut call sites visibly respect command-palette state, and scale PR icon geometry directly. Constraint: Verification remains CI-only for builds/tests; local direct builds are forbidden.\nRejected: Keep only a wrapper scaleEffect around PR icons | reviewers still flagged the icon internals as fixed-size.\nConfidence: high\nScope-risk: narrow\nTested: git diff --check; plutil -lint GhosttyTabs.xcodeproj/project.pbxproj; jq empty Resources/Localizable.xcstrings web/data/cmux.schema.json web/data/cmux-settings.schema.json\nNot-tested: Local build or unit tests, per instruction
The async persistence path must not call settings-store reload from its background disk queue. Split app.uiScale file writes from store reload so live zoom changes write cmux.json off the event path, then hop back to main for shared settings state and notifications. Also remove the redundant command-palette guard inside the already-guarded UI scale shortcut helper. Constraint: Builds and tests must run in CI only for this task.\nRejected: Keep using persistAppUIScale from the background queue | that method also mutates store state through reload.\nConfidence: high\nScope-risk: narrow\nTested: git diff --check; plutil -lint GhosttyTabs.xcodeproj/project.pbxproj; jq empty Resources/Localizable.xcstrings web/data/cmux.schema.json web/data/cmux-settings.schema.json\nNot-tested: Local build or unit tests, per instruction
The app-global key equivalent path must report whether the UI scale shortcut consumed the event so the activation build compiles and downstream key handling remains accurate. Constraint: Local builds and tests are intentionally left to CI for this branch. Confidence: high Scope-risk: narrow Tested: git diff --check; plutil -lint GhosttyTabs.xcodeproj/project.pbxproj; jq empty Resources/Localizable.xcstrings web/data/cmux.schema.json web/data/cmux-settings.schema.json Not-tested: Local xcodebuild/tests per branch instructions
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 `@Sources/KeyboardShortcutSettingsFileStore.swift`:
- Around line 166-169: persistAppUIScale currently calls reload() directly which
can run on a background queue; change it to dispatch the reload to the main
thread after the write. Specifically, inside persistAppUIScale (which calls
writeAppUIScale), wrap the reload() invocation in a main-thread dispatch (e.g.,
DispatchQueue.main.async or equivalent) so reload() always runs on the main
thread; keep the writeAppUIScale call unchanged.
🪄 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: a5482206-d772-4a2a-be97-c888f046dbe5
📒 Files selected for processing (9)
Sources/AppDelegate.swiftSources/ContentView.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/SessionIndexView.swiftSources/SettingsNavigation.swiftSources/TaskManagerWindowController.swiftSources/UIScaleSettings.swiftSources/cmuxApp.swiftcmuxTests/UIScaleSettingsTests.swift
The CI unit target caught test-only compile issues in the new UI scale coverage. The test helper now calls JSONCParser with its required label and polls the optional persisted value without double-unwrapping. Constraint: Local tests are not run on this branch; CircleCI is the test runner. Confidence: high Scope-risk: narrow Tested: git diff --check; plutil -lint GhosttyTabs.xcodeproj/project.pbxproj; jq empty Resources/Localizable.xcstrings web/data/cmux.schema.json web/data/cmux-settings.schema.json Not-tested: Local xcodebuild/tests per branch instructions
Review caught two remaining scale propagation edges: the settings-file persistence helper could reload off-main when used directly, and an equatable session icon omitted the environment scale from its equality check. The helper now schedules reload on the main queue for non-main callers, and the icon equality includes uiScaleFactor. Constraint: Do not run local tests/builds for this branch; use CI feedback. Confidence: high Scope-risk: narrow Tested: git diff --check; plutil -lint GhosttyTabs.xcodeproj/project.pbxproj; jq empty Resources/Localizable.xcstrings web/data/cmux.schema.json web/data/cmux-settings.schema.json Not-tested: Local xcodebuild/tests per branch instructions
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/SessionIndexView.swift (1)
2105-2116:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissed
cmuxFontconversion inloadingRow.Every other
TextinSectionPopoverViewwas switched to.cmuxFont(size:)so it followsuiScaleFactor, but the "Loading…" label here still uses the unscaled.font(.system(size: 11)). As a result, both call sites ofloadingRow(the empty-state spinner at line 1964 and the pagination sentinel at line 1986) will render at a fixed 11pt regardless of the user's UI scale, making the spinner row visually inconsistent with the rest of the popover at non-default scales.🎯 Proposed fix
private var loadingRow: some View { HStack(spacing: 6) { ProgressView().controlSize(.small) Text(String(localized: "sessionIndex.popover.loading", defaultValue: "Loading…")) - .font(.system(size: 11)) + .cmuxFont(size: 11) .foregroundColor(.secondary) Spacer(minLength: 0) }🤖 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 `@Sources/SessionIndexView.swift` around lines 2105 - 2116, The loadingRow HStack in SectionPopoverView uses .font(.system(size: 11)) for the "Loading…" Text, so it ignores the uiScaleFactor; replace that .font call by using .cmuxFont(size:) with the scaled size (11) to match other Texts (e.g., other uses of cmuxFont in SectionPopoverView) so the spinner row respects cmuxFont scaling and remains consistent at non-default UI scales; update the Text in loadingRow to use .cmuxFont(size: 11) and keep the existing .foregroundColor(.secondary) and layout intact.
🤖 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.
Outside diff comments:
In `@Sources/SessionIndexView.swift`:
- Around line 2105-2116: The loadingRow HStack in SectionPopoverView uses
.font(.system(size: 11)) for the "Loading…" Text, so it ignores the
uiScaleFactor; replace that .font call by using .cmuxFont(size:) with the scaled
size (11) to match other Texts (e.g., other uses of cmuxFont in
SectionPopoverView) so the spinner row respects cmuxFont scaling and remains
consistent at non-default UI scales; update the Text in loadingRow to use
.cmuxFont(size: 11) and keep the existing .foregroundColor(.secondary) and
layout intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 42434040-0306-4000-8fad-a9b89d3a774a
📒 Files selected for processing (3)
Sources/KeyboardShortcutSettingsFileStore.swiftSources/SessionIndexView.swiftcmuxTests/UIScaleSettingsTests.swift
The global chrome scale should apply consistently to popover content, including transient loading rows, so the remaining fixed system font is routed through the same cmuxFont environment helper as adjacent rows. Constraint: CodeRabbit found the latest head still had one fixed-size SwiftUI text modifier in SessionIndexView Rejected: Introduce a separate loading-row scale calculation | the existing cmuxFont helper already preserves the single environment-driven path Confidence: high Scope-risk: narrow Tested: git diff --check Not-tested: Local Swift tests or app build per task instruction to use CI only before final tagged reload
The live UI scale path now writes through the debounced settings-file writer, so keeping a second direct persistence API invites stale review concerns without adding behavior. Constraint: PR review flagged persistAppUIScale as unused after the background persistence path became the only caller-facing flow Rejected: Keep the wrapper for possible future callers | unused settings-store APIs make thread-affinity guarantees harder to audit Confidence: high Scope-risk: narrow Tested: git diff --check Not-tested: Local Swift tests or app build per task instruction to use CI only before final tagged reload
Contributor
Author
|
Greptile summary follow-up: the two UIScaleSettings persistence issues called out in the older summary were addressed in e10f9d4. UI scale mutations now enter through @mainactor set/zoom/reset APIs, pending-write state no longer uses volatile UserDefaults domains, and settings-file JSONC writes run through writeAppUIScaleOffMain() via an explicit utility-priority Task.detached boundary. |
…-3862-global-ui-font-size # Conflicts: # Sources/AppDelegate.swift # Sources/ContentView.swift
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 15a356f. Configure here.
lawrencecchen
added
the
stale-revisit
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
3 participants
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.
Closes #3862
Summary
Testing
Demo Video
Note
Medium Risk
Touches cross-cutting UI layout/typography and introduces new settings-file persistence/editing logic, which could cause regressions in sizing, shortcut routing, or config writes if edge cases are missed.
Overview
Adds a new global UI scaling setting (
app.uiScale) that is clamped, stored in UserDefaults, persisted tocmux.json, and injected via auiScaleFactorSwiftUI environment value from the main window root.Introduces configurable UI Zoom In/Out/Reset shortcuts and routes them through the app’s key-event handling (including AppKit key-equivalent fallbacks) while suppressing them when the command palette is effectively visible.
Applies UI scale throughout built-in chrome (sidebars, right sidebar controls, file explorer AppKit views, markdown web preview base font size, various headers/popovers) via scaled metrics/fonts, updates settings/search indexing to surface the new option, and adds full localization strings/aliases for the new UI scale and menu items.
Reviewed by Cursor Bugbot for commit 5da03c9. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds a global UI scale setting in
cmux.jsonwith app‑wide Zoom In/Out/Reset shortcuts. Scales built‑in chrome across the app independent of terminal font size or browser zoom (Linear 3862).New Features
app.uiScale(default 1.0, clamp 0.7–2.0) with a slider and customizable shortcuts (Cmd+Shift+=/−/0).uiScaleFactorinjected from app roots and applied across SwiftUI/AppKit chrome: sidebars, file explorer (row height/indent/icons/header), right‑sidebar mode bar/controls, settings, markdown base font size, session index and transcript popovers, task manager, popovers, and the shortcut hint pill.web/data/cmux.schema.json,web/data/cmux-shortcuts.ts); JSONC‑preserving settings editor; tests (UIScaleSettingsTests) cover cmux.json round‑trip, environment propagation, and clamp/reset via shortcuts.Bug Fixes
uiScaleFactor.Written for commit 5da03c9. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
UI Improvements
Localization
Tests