feat: add UI zoom scaling for all non-terminal UI elements#1074
feat: add UI zoom scaling for all non-terminal UI elements#1074moyashin63 wants to merge 4 commits intomanaflow-ai:mainfrom
Conversation
Rename SidebarZoomMetrics to UIZoomMetrics and apply zoom scaling to titlebar, command palette, browser address bar, search overlays, notifications, markdown panel, feedback dialog, workspace, and update UI. Search overlay counter text now also scales with zoom. Sidebar minimum width is now a reactive instance property tied to @AppStorage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
At scale=2.0, minimumSidebarWidth (186*2=372pt) could exceed maxSidebarWidth (availableWidth/3), making minimum > maximum and locking the sidebar width. Remove scaling from minimumSidebarWidth since sidebar contents already scale individually. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@moyashin63 is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds a UI zoom feature: new UIZoomMetrics utility, keyboard shortcut detection for UI zoom in AppDelegate, persisted zoom scale, and widespread replacement of hard-coded sizes with zoom-aware metrics across SwiftUI views and session logic. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant AppDelegate as AppDelegate
participant UserDefaults as UserDefaults
participant Views as SwiftUI_Views
User->>AppDelegate: Press Cmd+Shift (+/= / - / 0)
AppDelegate->>AppDelegate: uiZoomShortcutAction(...) → determine action
AppDelegate->>UserDefaults: write UIZoomMetrics.appStorageKey (new scale)
AppDelegate-->>User: return handled (short-circuit)
UserDefaults->>Views: `@AppStorage` publishes updated scale
Views->>Views: UIZoomMetrics compute scaled metrics
Views-->>User: UI re-renders with scaled dimensions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 14 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="Sources/UIZoomMetrics.swift">
<violation number="1" location="Sources/UIZoomMetrics.swift:17">
P2: `effectiveScale()` returns the raw stored value without calling `clamped()`, so it can return values outside the valid [0.5, 2.0] range if UserDefaults contains a corrupt or manually-set value. Wrap the return in `clamped()` for defensive consistency.</violation>
</file>
<file name="Sources/ContentView.swift">
<violation number="1" location="Sources/ContentView.swift:2035">
P2: Hardcoded `12` in the sidebar-visible titlebar leading padding is not zoom-scaled, unlike every other padding value in the same expression. At extreme zoom levels (0.5x or 2.0x) this will look inconsistent.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| .padding(.trailing, 8) | ||
| .frame(height: UIZoomMetrics.titlebarHeight(uiZoomScale)) | ||
| .padding(.top, UIZoomMetrics.titlebarTopPadding(uiZoomScale)) | ||
| .padding(.leading, (isFullScreen && !sidebarState.isVisible) ? UIZoomMetrics.titlebarHorizontalPadding(uiZoomScale) : (sidebarState.isVisible ? 12 : titlebarLeadingInset + CGFloat(debugTitlebarLeadingExtra))) |
There was a problem hiding this comment.
P2: Hardcoded 12 in the sidebar-visible titlebar leading padding is not zoom-scaled, unlike every other padding value in the same expression. At extreme zoom levels (0.5x or 2.0x) this will look inconsistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/ContentView.swift, line 2035:
<comment>Hardcoded `12` in the sidebar-visible titlebar leading padding is not zoom-scaled, unlike every other padding value in the same expression. At extreme zoom levels (0.5x or 2.0x) this will look inconsistent.</comment>
<file context>
@@ -2019,18 +2022,18 @@ struct ContentView: View {
- .padding(.trailing, 8)
+ .frame(height: UIZoomMetrics.titlebarHeight(uiZoomScale))
+ .padding(.top, UIZoomMetrics.titlebarTopPadding(uiZoomScale))
+ .padding(.leading, (isFullScreen && !sidebarState.isVisible) ? UIZoomMetrics.titlebarHorizontalPadding(uiZoomScale) : (sidebarState.isVisible ? 12 : titlebarLeadingInset + CGFloat(debugTitlebarLeadingExtra)))
+ .padding(.trailing, UIZoomMetrics.titlebarHorizontalPadding(uiZoomScale))
}
</file context>
| .padding(.leading, (isFullScreen && !sidebarState.isVisible) ? UIZoomMetrics.titlebarHorizontalPadding(uiZoomScale) : (sidebarState.isVisible ? 12 : titlebarLeadingInset + CGFloat(debugTitlebarLeadingExtra))) | |
| .padding(.leading, (isFullScreen && !sidebarState.isVisible) ? UIZoomMetrics.titlebarHorizontalPadding(uiZoomScale) : (sidebarState.isVisible ? UIZoomMetrics.titlebarHorizontalPadding(uiZoomScale) : titlebarLeadingInset + CGFloat(debugTitlebarLeadingExtra))) |
There was a problem hiding this comment.
Won't fix. This 12 is a structural gap between the sidebar edge and the titlebar text — not a content element. The other branch (titlebarLeadingInset, ~78pt) is an OS-measured traffic-light width, also unscaled. Scaling this margin to 24pt at 2.0x would create an unnaturally large gap between the sidebar and the title.
Greptile SummaryThis PR introduces VSCode-style UI zoom ( Key changes:
Issues found:
Confidence Score: 3/5
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[NSEvent keyDown] --> B{Extract flags\nchars, keyCode}
B --> C{uiZoomShortcutAction\nCmd+Shift+= / - / 0}
C -->|Match| D[Read UIZoomMetrics\n.effectiveScale]
D --> E{Action?}
E -->|zoomIn| F[current + step]
E -->|zoomOut| G[current - step]
E -->|reset| H[defaultScale = 1.0]
F --> I[UIZoomMetrics.clamped\n0.5x – 2.0x]
G --> I
H --> I
I --> J[UserDefaults.set\nuiZoomScale]
J --> K[return true\n⚠️ event consumed]
C -->|No match| L{browserZoomShortcutAction\nCmd+Shift+= / - / 0}
L -->|Match| M[Zoom browser\nwebview content]
L -->|No match| N[Other handlers…]
K -.->|⚠️ Never reached\nif Cmd+Shift+=/−/0| L
style K fill:#f99,stroke:#c00
style L fill:#fdd,stroke:#c00
Last reviewed commit: e3aefbd |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| .padding(.trailing, 8) | ||
| .frame(height: UIZoomMetrics.titlebarHeight(uiZoomScale)) | ||
| .padding(.top, UIZoomMetrics.titlebarTopPadding(uiZoomScale)) | ||
| .padding(.leading, (isFullScreen && !sidebarState.isVisible) ? UIZoomMetrics.titlebarHorizontalPadding(uiZoomScale) : (sidebarState.isVisible ? 12 : titlebarLeadingInset + CGFloat(debugTitlebarLeadingExtra))) |
There was a problem hiding this comment.
Titlebar leading padding not consistently zoom-scaled
Only the fullscreen + hidden-sidebar branch uses the zoom-scaled titlebarHorizontalPadding. The two other cases (sidebar visible → hardcoded 12; sidebar hidden, not fullscreen → titlebarLeadingInset + debugTitlebarLeadingExtra) remain unscaled. This means the titlebar leading inset stays fixed regardless of zoom when the sidebar is shown or when in windowed mode.
If consistent zoom scaling is intended across all layout states, apply the zoom factor to all three cases:
.padding(.leading, (isFullScreen && !sidebarState.isVisible)
? UIZoomMetrics.titlebarHorizontalPadding(uiZoomScale)
: (sidebarState.isVisible
? UIZoomMetrics.titlebarHorizontalPadding(uiZoomScale) * 1.5 // or a dedicated metric
: titlebarLeadingInset + CGFloat(debugTitlebarLeadingExtra)))If the asymmetry is intentional, add a code comment explaining why those branches remain unscaled.
There was a problem hiding this comment.
Won't fix. Same reasoning as the other thread on this line: the 12 is a structural margin between the sidebar edge and titlebar content, not a scalable UI element. The titlebarLeadingInset (~78pt) in the third branch is OS-measured (traffic light width + accessories) and also unscaled. Scaling structural gaps creates unnatural spacing at non-1.0x zoom.
| static func minimumSidebarWidth(_ scale: Double) -> Double { | ||
| baseMinimumSidebarWidth | ||
| } |
There was a problem hiding this comment.
Scale parameter accepted but silently ignored
minimumSidebarWidth accepts a scale argument but always returns the constant baseMinimumSidebarWidth. While this is intentional (avoiding minimum > maximum at high zoom), the misleading signature will confuse future maintainers who expect the scale to be applied. Consider adding a clarifying comment:
/// Always returns the base width regardless of scale to prevent minimum > maximum
/// sidebar-width violations at high zoom levels (e.g. 2× scale would push minimum
/// to 372 pt, exceeding the 1/3-window maximum on typical screens).
static func minimumSidebarWidth(_ scale: Double) -> Double {
baseMinimumSidebarWidth
}There was a problem hiding this comment.
Won't fix. The scale parameter is intentionally accepted but unused for API consistency — all metrics functions share the same (_ scale: Double) -> CGFloat signature. Scaling the minimum sidebar width would break the sidebar at extreme zoom levels (e.g., 0.5x → 93pt minimum, too narrow for sidebar content). The parameter is retained so future scaling can be added without an API change.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
Sources/WorkspaceContentView.swift (1)
391-408:⚠️ Potential issue | 🟠 MajorLocalize the empty-state strings in this hunk.
"Empty Panel","Terminal", and"Browser"are user-facing strings in touched UI and should useString(localized:).Proposed fix
- Text("Empty Panel") + Text(String(localized: "workspace.emptyPanel.title", defaultValue: "Empty Panel")) - title: "Terminal", + title: String(localized: "workspace.emptyPanel.terminal", defaultValue: "Terminal"), - title: "Browser", + title: String(localized: "workspace.emptyPanel.browser", defaultValue: "Browser"),As per coding guidelines, "All user-facing strings must be localized using
String(localized: "key.name", defaultValue: "English text")for every string shown in the UI".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/WorkspaceContentView.swift` around lines 391 - 408, Replace the hard-coded user-facing strings in the empty-state UI with localized variants: change the Text("Empty Panel") usage and the emptyPaneActionButton title arguments ("Terminal" and "Browser") to use String(localized:..., defaultValue:...) per guidelines; update the Text initializer and the emptyPaneActionButton calls (the ones that pass title: "Terminal" and title: "Browser") to use String(localized: "workspace.emptyPanel.title", defaultValue: "Empty Panel") and similar keys like "workspace.emptyPanel.terminal" / "workspace.emptyPanel.browser" with appropriate defaultValue text so all displayed UI strings are localized.Sources/Update/UpdatePopoverView.swift (1)
8-42:⚠️ Potential issue | 🟠 MajorThis only scales the shell, not the popover content.
uiZoomScaleonly drives the outer width here. The subviews below still hard-code paddings and font sizes (16,13,12,11,10, etc.), so 0.5× makes the popover too narrow for unchanged content and 2.0× leaves a lot of whitespace around still-tiny text. Please thread theUIZoomMetrics.updatePopover*values through the child views as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Update/UpdatePopoverView.swift` around lines 8 - 42, The popover only applies UIZoomMetrics.updatePopoverWidth(uiZoomScale) to the outer frame while child views still use hard-coded paddings and font sizes; fix by threading the zoom scale or the computed metrics (use UIZoomMetrics.updatePopover* helpers) into each child view initializer (PermissionRequestView, CheckingView, UpdateAvailableView, DownloadingView, ExtractingView, InstallingView, NotFoundView, UpdateErrorView) and update those view implementations to use the passed scale/metrics for all paddings, spacings and font sizes instead of fixed numeric literals so the entire popover content scales consistently.Sources/Find/SurfaceSearchOverlay.swift (1)
30-116:⚠️ Potential issue | 🟠 MajorThe find overlay still isn't actually zoomed end-to-end.
These changes scale the wrapper geometry, but the real controls are still fixed-size:
SearchTextFieldRepresentable.makeNSViewkeeps the field atNSFont.systemFontSize, andSearchButtonStylestill hard-codes the button height/radius. At 2× the bar grows while the typed text and action buttons stay tiny.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Find/SurfaceSearchOverlay.swift` around lines 30 - 116, The overlay geometry is scaled but the controls remain fixed-size; update SearchTextFieldRepresentable.makeNSView to use the UI zoom metrics for its font and paddings (e.g., use UIZoomMetrics.searchFieldFontSize(uiZoomScale) or equivalent instead of NSFont.systemFontSize) and make SearchButtonStyle derive its height, corner radius and image/font sizes from UIZoomMetrics (e.g., searchButtonHeight, searchButtonCornerRadius, searchButtonImageSize) so the text field glyphs and buttons scale consistently with uiZoomScale; ensure the representable receives the uiZoomScale (or resolves the same UIZoomMetrics key) so makeNSView can compute sizes and the button style reads the same scale values.Sources/ContentView.swift (4)
1603-1649:⚠️ Potential issue | 🟠 MajorRe-clamp the sidebar when
uiZoomScalechanges.
minimumSidebarWidthis zoom-dependent now, butsidebarWidth/sidebarState.persistedWidthare only sanitized on appear, drag, persistence updates, and window resize. If the user zooms in with a narrow sidebar, the live width can sit below the new minimum until some unrelated resize path runs.Suggested follow-up
+ view = AnyView(view.onChange(of: uiZoomScale) { _ in + let sanitized = normalizedSidebarWidth(sidebarWidth) + if abs(sidebarWidth - sanitized) > 0.5 { + sidebarWidth = sanitized + } + if abs(sidebarState.persistedWidth - sanitized) > 0.5 { + sidebarState.persistedWidth = sanitized + } + updateSidebarResizerBandState() + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 1603 - 1649, The minimumSidebarWidth now depends on uiZoomScale but we never re-check sidebar constraints when uiZoomScale changes; add logic to re-clamp the live and persisted sidebar width whenever uiZoomScale updates by calling clampSidebarWidthIfNeeded (and updating persisted width/session state if you also store persistedWidth like sidebarState.persistedWidth) from an onChange / observer of uiZoomScale so the current sidebarWidth is normalized via normalizedSidebarWidth and enclosure logic in clampSidebarWidthIfNeeded is applied immediately after zoom changes.
2956-3053:⚠️ Potential issue | 🟡 MinorRename mode still snaps the command palette back to fixed sizing.
The list view is zoom-aware now, but
commandPaletteRenameInputViewandcommandPaletteRenameConfirmViewbelow still hardcode 13/11pt fonts and fixed paddings. Entering rename mode will visibly undo the zoom setting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 2956 - 3053, The rename UI still uses fixed 13/11pt fonts and fixed paddings causing it to ignore zoom; update commandPaletteRenameInputView and commandPaletteRenameConfirmView to use the same UIZoomMetrics helpers and the local uiZoomScale (e.g., replace hardcoded font sizes with UIZoomMetrics.paletteFieldFontSize/uiZoomScale or paletteResultFontSize, replace fixed horizontal/vertical paddings with UIZoomMetrics.paletteFieldHPadding/…/paletteResultVPadding, and use paletteTrailingFontSize/paletteTrailingHPadding/paletteTrailingVPadding and paletteTrailingCornerRadius where applicable) so the rename views match the zoom-aware sizing and styling used elsewhere (preserve font weights and semantics like .secondary foregrounds and rounded backgrounds).
9286-9300:⚠️ Potential issue | 🟡 MinorScale the shortcut-hint slot width with the pill.
The hint pill's font and padding now grow with
uiZoomScale, butworkspaceHintSlotWidthstill comes fromSidebarWorkspaceShortcutHintMetrics' fixed 10pt measurement. At higher zoom the pill can intrude into the title/close-button space.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 9286 - 9300, workspaceHintSlotWidth is still a fixed 10pt and doesn’t scale with uiZoomScale, so the shortcut pill (whose font and padding use UIZoomMetrics) can overlap the title/close-button; update where workspaceHintSlotWidth is defined or used so the slot width scales with the pill by computing it from the same UIZoomMetrics values (use UIZoomMetrics.subtitleFontSize(uiZoomScale), UIZoomMetrics.shortcutHintHorizontalPadding(uiZoomScale) and UIZoomMetrics.shortcutHintVerticalPadding(uiZoomScale) or a simple multiplier of the current baseline 10pt like 10 * uiZoomScale) and replace the fixed width passed to .frame(width: workspaceHintSlotWidth, ...) with this scaled value so the pill never intrudes on neighboring UI.
8121-8253:⚠️ Potential issue | 🟡 MinorThe feedback sheet's textarea is still fixed at 12pt.
Only the surrounding SwiftUI labels scale here.
FeedbackComposerMessageEditorViewlater in the file still hardcodes 12pt fonts for both theNSTextViewand placeholder, so the main typing surface stays tiny at larger zoom levels.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 8121 - 8253, The message editor currently hardcodes 12pt in FeedbackComposerMessageEditorView for the NSTextView and its placeholder, so the typing surface doesn't respond to UI zoom; update FeedbackComposerMessageEditorView (used by FeedbackComposerMessageEditor) to accept or access uiZoomScale and replace the hardcoded 12pt with UIZoomMetrics.feedbackFontSize(uiZoomScale) for the NSTextView.font and for the placeholder/attributed string so the editor text and placeholder scale consistently with surrounding labels. Also ensure any font-weight/line-height settings applied to the NSTextView or placeholder are preserved when switching to the dynamic size.
🧹 Nitpick comments (1)
Sources/AppDelegate.swift (1)
1166-1204: Factor the zoom key parsing into the existing browser zoom matcher.This helper duplicates the same key classification matrix as
browserZoomShortcutAction(...). Keeping two copies makes keypad/layout fixes easy to miss in one path. A shared parser with separate modifier policies would be safer long-term.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 1166 - 1204, The zoom key classification in uiZoomShortcutAction duplicates the key-mapping logic in browserZoomShortcutAction; refactor by extracting the shared key parsing into a single helper (or reuse browserZoomShortcutAction's parsing) so uiZoomShortcutAction only enforces its modifier policy and then asks that shared parser for .zoomIn/.zoomOut/.reset based on browserZoomShortcutKeyCandidates(chars:literalChars:keyCode:) and keyCode; update uiZoomShortcutAction to call the shared parser (or browserZoomShortcutAction) instead of rechecking "=", "+", "-", "_", "0" and keypad keyCodes so keypad/layout fixes are centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 7523-7542: The UI zoom shortcut (uiZoomShortcutAction) is never
reached because shouldConsumeShortcutWhileCommandPaletteVisible prevents
handling when the command palette is visible; modify the shortcut handling so
uiZoomShortcutAction(flags:chars:keyCode:literalChars:) is evaluated before or
exempted from shouldConsumeShortcutWhileCommandPaletteVisible(...)—i.e., check
uiZoomShortcutAction first (or add a special-case that skips the command-palette
guard for zoom actions) and then apply the existing UIZoomMetrics logic
(clamped/step/defaultScale and UserDefaults.standard.set with
UIZoomMetrics.appStorageKey) so Cmd+Shift+=/-/0 works while the palette is open.
- Around line 7523-7542: Add DEBUG-only dlog() tracing to the UI zoom shortcut
path handled by uiZoomShortcutAction: inside the if let block that computes
current/next (referencing UIZoomMetrics and uiZoomShortcutAction) add `#if` DEBUG
/ `#endif` guarded dlog() calls that record the resolved uiZoomAction
(zoomIn/zoomOut/reset), the previous scale (current) and the computed next scale
before calling UserDefaults.standard.set; use the same dlog() style/payload as
the adjacent browser zoom path so the event appears in the unified debug event
log.
In `@Sources/Find/BrowserSearchOverlay.swift`:
- Around line 14-17: The GeometryReader currently captures barSize only in its
onAppear, so when uiZoomScale changes the stored barSize (used for drag
snapping) becomes stale; update the code to recompute barSize whenever
uiZoomScale changes by reacting to uiZoomScale (e.g., use onChange(of:
uiZoomScale) to recalc and set barSize or derive barSize from geometry.size and
uiZoomScale as a computed property), and apply the same fix to the other
GeometryReader usages that set barSize (referenced by the barSize variable and
the GeometryReader/onAppear blocks).
- Around line 29-38: The TextField placeholder in BrowserSearchOverlay is using
a bare string literal "Search"; update it to use the String(localized:) API so
it pulls from the string catalog (e.g., replace the literal in the TextField
initializer that currently reads TextField("Search", text: $searchState.needle)
with a localized placeholder using String(localized: ...)). Ensure the localized
key/value is added to the appropriate .strings file and keep the binding to
searchState.needle and existing modifiers unchanged.
In `@Sources/NotificationsPage.swift`:
- Line 45: NotificationsPage only scales container padding via
UIZoomMetrics.notificationContainerPadding(uiZoomScale) but not the content;
update the views so the zoom factor applies to all visual metrics. Pass
uiZoomScale into NotificationRow and any ShortcutBadge so they use scaled fonts,
paddings, cornerRadius and spacing (e.g. multiply base font sizes, padding
constants and corner radii by uiZoomScale inside NotificationRow and
ShortcutBadge). Also apply the same scaling to the header title and button
labels in NotificationsPage (either by using scaled font sizes or a
.scaleEffect(uiZoomScale) on those label views). Ensure all references to fixed
constants are replaced with UIZoomMetrics helpers or multiplied by uiZoomScale
so the entire page content (not just outer padding) respects the zoom.
In `@Sources/Panels/BrowserPanelView.swift`:
- Around line 3228-3239: The popup layout reads uiZoomScale but the actual
content (row text font, badge font, and badge paddings) still uses fixed sizes;
update the popup rendering to derive those sizes from the same uiZoomScale via
UIZoomMetrics (e.g., add/use UIZoomMetrics methods for badgeFontSize,
badgePadding, and rowTextFontSize) and replace hard-coded font sizes/paddings in
the popup row/badge rendering with calls that use the existing uiZoomScale
property so text and badges scale together with popupCornerRadius,
singleLineRowHeight, rowSpacing, and maxPopupHeight.
- Around line 239-246: The omnibar visuals are being scaled but the text and
symbol sizes remain hard-coded at 12pt; update
OmnibarTextFieldRepresentable.makeNSView to use a zoomed font size from
UIZoomMetrics (e.g., replace the fixed 12pt font with a UIZoomMetrics-derived
font using the uiZoomScale AppStorage value) and update addressBarButtonBar to
set its SF Symbol/image point size from UIZoomMetrics (use
devToolsButtonIconSize/addressBarButtonSize or a new
UIZoomMetrics.iconSize(uiZoomScale) metric) so labels and icons scale
consistently with omnibarPillCornerRadius, addressBarButtonHitSize, and other
metrics. Ensure you reference uiZoomScale when constructing fonts and images so
all omnibar chrome (text and symbols) visually zooms together.
In `@Sources/Panels/MarkdownPanelView.swift`:
- Line 15: The markdown content styles still use fixed sizes: update
cmuxMarkdownTheme and fileUnavailableView to multiply their font sizes,
lineSpacing, and padding values by the stored zoom factor (uiZoomScale from
`@AppStorage`(UIZoomMetrics.appStorageKey) in MarkdownPanelView) so the rendered
markdown scales with chrome; locate and replace hardcoded Font, padding, and
spacing constants in cmuxMarkdownTheme and the fileUnavailableView body to
derive values using uiZoomScale (e.g., baseSize * uiZoomScale) and ensure any
Text or VStack/HStack paddings use the scaled values as well.
---
Outside diff comments:
In `@Sources/ContentView.swift`:
- Around line 1603-1649: The minimumSidebarWidth now depends on uiZoomScale but
we never re-check sidebar constraints when uiZoomScale changes; add logic to
re-clamp the live and persisted sidebar width whenever uiZoomScale updates by
calling clampSidebarWidthIfNeeded (and updating persisted width/session state if
you also store persistedWidth like sidebarState.persistedWidth) from an onChange
/ observer of uiZoomScale so the current sidebarWidth is normalized via
normalizedSidebarWidth and enclosure logic in clampSidebarWidthIfNeeded is
applied immediately after zoom changes.
- Around line 2956-3053: The rename UI still uses fixed 13/11pt fonts and fixed
paddings causing it to ignore zoom; update commandPaletteRenameInputView and
commandPaletteRenameConfirmView to use the same UIZoomMetrics helpers and the
local uiZoomScale (e.g., replace hardcoded font sizes with
UIZoomMetrics.paletteFieldFontSize/uiZoomScale or paletteResultFontSize, replace
fixed horizontal/vertical paddings with
UIZoomMetrics.paletteFieldHPadding/…/paletteResultVPadding, and use
paletteTrailingFontSize/paletteTrailingHPadding/paletteTrailingVPadding and
paletteTrailingCornerRadius where applicable) so the rename views match the
zoom-aware sizing and styling used elsewhere (preserve font weights and
semantics like .secondary foregrounds and rounded backgrounds).
- Around line 9286-9300: workspaceHintSlotWidth is still a fixed 10pt and
doesn’t scale with uiZoomScale, so the shortcut pill (whose font and padding use
UIZoomMetrics) can overlap the title/close-button; update where
workspaceHintSlotWidth is defined or used so the slot width scales with the pill
by computing it from the same UIZoomMetrics values (use
UIZoomMetrics.subtitleFontSize(uiZoomScale),
UIZoomMetrics.shortcutHintHorizontalPadding(uiZoomScale) and
UIZoomMetrics.shortcutHintVerticalPadding(uiZoomScale) or a simple multiplier of
the current baseline 10pt like 10 * uiZoomScale) and replace the fixed width
passed to .frame(width: workspaceHintSlotWidth, ...) with this scaled value so
the pill never intrudes on neighboring UI.
- Around line 8121-8253: The message editor currently hardcodes 12pt in
FeedbackComposerMessageEditorView for the NSTextView and its placeholder, so the
typing surface doesn't respond to UI zoom; update
FeedbackComposerMessageEditorView (used by FeedbackComposerMessageEditor) to
accept or access uiZoomScale and replace the hardcoded 12pt with
UIZoomMetrics.feedbackFontSize(uiZoomScale) for the NSTextView.font and for the
placeholder/attributed string so the editor text and placeholder scale
consistently with surrounding labels. Also ensure any font-weight/line-height
settings applied to the NSTextView or placeholder are preserved when switching
to the dynamic size.
In `@Sources/Find/SurfaceSearchOverlay.swift`:
- Around line 30-116: The overlay geometry is scaled but the controls remain
fixed-size; update SearchTextFieldRepresentable.makeNSView to use the UI zoom
metrics for its font and paddings (e.g., use
UIZoomMetrics.searchFieldFontSize(uiZoomScale) or equivalent instead of
NSFont.systemFontSize) and make SearchButtonStyle derive its height, corner
radius and image/font sizes from UIZoomMetrics (e.g., searchButtonHeight,
searchButtonCornerRadius, searchButtonImageSize) so the text field glyphs and
buttons scale consistently with uiZoomScale; ensure the representable receives
the uiZoomScale (or resolves the same UIZoomMetrics key) so makeNSView can
compute sizes and the button style reads the same scale values.
In `@Sources/Update/UpdatePopoverView.swift`:
- Around line 8-42: The popover only applies
UIZoomMetrics.updatePopoverWidth(uiZoomScale) to the outer frame while child
views still use hard-coded paddings and font sizes; fix by threading the zoom
scale or the computed metrics (use UIZoomMetrics.updatePopover* helpers) into
each child view initializer (PermissionRequestView, CheckingView,
UpdateAvailableView, DownloadingView, ExtractingView, InstallingView,
NotFoundView, UpdateErrorView) and update those view implementations to use the
passed scale/metrics for all paddings, spacings and font sizes instead of fixed
numeric literals so the entire popover content scales consistently.
In `@Sources/WorkspaceContentView.swift`:
- Around line 391-408: Replace the hard-coded user-facing strings in the
empty-state UI with localized variants: change the Text("Empty Panel") usage and
the emptyPaneActionButton title arguments ("Terminal" and "Browser") to use
String(localized:..., defaultValue:...) per guidelines; update the Text
initializer and the emptyPaneActionButton calls (the ones that pass title:
"Terminal" and title: "Browser") to use String(localized:
"workspace.emptyPanel.title", defaultValue: "Empty Panel") and similar keys like
"workspace.emptyPanel.terminal" / "workspace.emptyPanel.browser" with
appropriate defaultValue text so all displayed UI strings are localized.
---
Nitpick comments:
In `@Sources/AppDelegate.swift`:
- Around line 1166-1204: The zoom key classification in uiZoomShortcutAction
duplicates the key-mapping logic in browserZoomShortcutAction; refactor by
extracting the shared key parsing into a single helper (or reuse
browserZoomShortcutAction's parsing) so uiZoomShortcutAction only enforces its
modifier policy and then asks that shared parser for .zoomIn/.zoomOut/.reset
based on browserZoomShortcutKeyCandidates(chars:literalChars:keyCode:) and
keyCode; update uiZoomShortcutAction to call the shared parser (or
browserZoomShortcutAction) instead of rechecking "=", "+", "-", "_", "0" and
keypad keyCodes so keypad/layout fixes are centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5cb2742-42b9-4247-adb9-938e3592c11c
📒 Files selected for processing (14)
GhosttyTabs.xcodeproj/project.pbxprojSources/AppDelegate.swiftSources/ContentView.swiftSources/Find/BrowserSearchOverlay.swiftSources/Find/SurfaceSearchOverlay.swiftSources/NotificationsPage.swiftSources/Panels/BrowserPanelView.swiftSources/Panels/MarkdownPanelView.swiftSources/SessionPersistence.swiftSources/UIZoomMetrics.swiftSources/Update/UpdatePill.swiftSources/Update/UpdatePopoverView.swiftSources/WorkspaceContentView.swiftvendor/bonsplit
| // UI zoom: Cmd+Shift+=/Cmd+Shift+-/Cmd+Shift+0 | ||
| if let uiZoomAction = uiZoomShortcutAction( | ||
| flags: flags, | ||
| chars: chars, | ||
| keyCode: event.keyCode, | ||
| literalChars: event.characters | ||
| ) { | ||
| let current = UIZoomMetrics.effectiveScale() | ||
| let next: Double | ||
| switch uiZoomAction { | ||
| case .zoomIn: | ||
| next = UIZoomMetrics.clamped(current + UIZoomMetrics.step) | ||
| case .zoomOut: | ||
| next = UIZoomMetrics.clamped(current - UIZoomMetrics.step) | ||
| case .reset: | ||
| next = UIZoomMetrics.defaultScale | ||
| } | ||
| UserDefaults.standard.set(next, forKey: UIZoomMetrics.appStorageKey) | ||
| return true | ||
| } |
There was a problem hiding this comment.
This shortcut is unreachable while the command palette is open.
Line 6999 returns early for almost every Command shortcut while the palette is visible, so this block never runs for Cmd+Shift+=/-/0 in that state. That leaves part of the “all non-terminal UI elements” zoom flow non-functional. Please exempt UI zoom from shouldConsumeShortcutWhileCommandPaletteVisible(...) or evaluate uiZoomShortcutAction(...) before that guard.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 7523 - 7542, The UI zoom shortcut
(uiZoomShortcutAction) is never reached because
shouldConsumeShortcutWhileCommandPaletteVisible prevents handling when the
command palette is visible; modify the shortcut handling so
uiZoomShortcutAction(flags:chars:keyCode:literalChars:) is evaluated before or
exempted from shouldConsumeShortcutWhileCommandPaletteVisible(...)—i.e., check
uiZoomShortcutAction first (or add a special-case that skips the command-palette
guard for zoom actions) and then apply the existing UIZoomMetrics logic
(clamped/step/defaultScale and UserDefaults.standard.set with
UIZoomMetrics.appStorageKey) so Cmd+Shift+=/-/0 works while the palette is open.
Add DEBUG tracing for the new UI zoom shortcut path.
The adjacent browser zoom path logs match/dispatch in DEBUG, but this new key route does not. Please add dlog() calls behind #if DEBUG so zoom shortcut routing stays debuggable.
As per coding guidelines, "All debug events (keys, mouse, focus, splits, tabs) must be logged to the unified debug event log in DEBUG builds using the dlog() free function".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 7523 - 7542, Add DEBUG-only dlog()
tracing to the UI zoom shortcut path handled by uiZoomShortcutAction: inside the
if let block that computes current/next (referencing UIZoomMetrics and
uiZoomShortcutAction) add `#if` DEBUG / `#endif` guarded dlog() calls that record
the resolved uiZoomAction (zoomIn/zoomOut/reset), the previous scale (current)
and the computed next scale before calling UserDefaults.standard.set; use the
same dlog() style/payload as the adjacent browser zoom path so the event appears
in the unified debug event log.
There was a problem hiding this comment.
DEBUG tracing addressed in 3c2b1a5 — added dlog("uiZoom.\(uiZoomAction) current=\(current) next=\(next)").
Regarding the command palette guard: shouldConsumeShortcutWhileCommandPaletteVisible does block Cmd+Shift shortcuts when the palette is visible, so UI zoom won't work while the palette is open. This is a minor edge case (unlikely to zoom while typing in the palette) but will be addressed separately.
There was a problem hiding this comment.
Oops, something went wrong! Please try again later. 🐰 💔
There was a problem hiding this comment.
Won't fix. This is intentional. shouldConsumeShortcutWhileCommandPaletteVisible (line 1078) guards .command flag, then consumes all Cmd shortcuts except an explicit allow-list (Cmd+A/C/V/X/Z/Y, arrows, Delete) to protect the text input field. UI zoom shortcuts (Cmd+Shift+=/−/0) being blocked while the command palette is open is correct behavior — the palette is a text input context and should not fire unrelated shortcuts. Users close the palette first, then zoom.
There was a problem hiding this comment.
@moyashin63 Acknowledged. Thanks for confirming the intent here and for adding the DEBUG trace — I see the dlog("uiZoom.(…) current=… next=…") in place.
I’ll treat the “UI zoom is intentionally blocked while the command palette is open” behavior as by-design for cmux and won’t flag it again.
Optional (no action required): consider dropping a brief inline comment near shouldConsumeShortcutWhileCommandPaletteVisible to document that UI zoom is deliberately consumed while the palette is visible, to preempt future refactors.
✏️ Learnings added
Learnt from: moyashin63
Repo: manaflow-ai/cmux PR: 1074
File: Sources/AppDelegate.swift:7523-7545
Timestamp: 2026-03-09T01:38:24.337Z
Learning: In manaflow-ai/cmux (Sources/AppDelegate.swift), shouldConsumeShortcutWhileCommandPaletteVisible(...) intentionally consumes most Command shortcuts while the command palette is visible to protect its text input. As a result, UI zoom shortcuts (Cmd+Shift+= / Cmd+Shift+− / Cmd+Shift+0) must not fire while the palette is open. Do not reorder handlers (e.g., uiZoomShortcutAction(...)) to bypass this guard; users close the palette before zooming.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 762
File: Sources/ContentView.swift:5784-5788
Timestamp: 2026-03-06T02:00:30.333Z
Learning: In manaflow-ai/cmux (Sources/ContentView.swift), ShortcutHintModifierPolicy.shouldShowHints is Command-only: it returns true only when modifierFlags normalized to [.command]. Control-hold must not show sidebar/titlebar shortcut hints, and close-button suppression in TabItemView must remain tied to showsModifierShortcutHints (Command hold) or the always-show debug flag.
Learnt from: SuperManfred
Repo: manaflow-ai/cmux PR: 803
File: Sources/Panels/BrowserPopupWindowController.swift:36-38
Timestamp: 2026-03-04T05:11:56.373Z
Learning: In Sources/Panels/BrowserPopupWindowController.swift and Sources/Panels/BrowserPanel.swift, `webView.isInspectable = true` (guarded by `#available(macOS 13.3, *)`) is intentionally enabled in all builds — not just DEBUG — because cmux is a developer tool and full Web Inspector access is desired in production builds as well. Do not flag this as a security concern.
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/ContentView.swift:3194-3196
Timestamp: 2026-03-04T14:05:48.668Z
Learning: In manaflow-ai/cmux (PR `#819`), Sources/ContentView.swift: The command palette’s external window labels intentionally use the global window index from the full orderedSummaries (index + 1), matching the Window menu in AppDelegate. Do not reindex after filtering out the current window to avoid mismatches (“Window 2” for an external window is expected).
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/AppDelegate.swift:3491-3493
Timestamp: 2026-03-04T14:06:12.296Z
Learning: In PRs affecting this repository, limit the scope of localization (i18n) changes to Japanese translations for the file Sources/AppDelegate.swift. Do not include UX enhancements (e.g., preferring workspace.customTitle in workspaceDisplayName() or altering move-target labels) in this PR. Open a separate follow-up issue to address any UX-related changes to avoid scope creep and keep localization review focused.
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/AppDelegate.swift:0-0
Timestamp: 2026-03-04T14:05:42.574Z
Learning: Guideline: In Swift files (cmux project), when handling pluralized strings, prefer using localization keys with the ICU-style plural forms .one and .other. For example, use keys like statusMenu.unreadCount.one for the singular case (1) and statusMenu.unreadCount.other for all other counts, and similarly for statusMenu.tooltip.unread.one/other. Rationale: ensures correct pluralization across locales and makes localization keys explicit. Review code to ensure any unread count strings and related tooltips follow this .one/.other key pattern and verify the correct value is chosen based on the count.
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 954
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-03-05T22:04:34.712Z
Learning: Adopt the convention: for health/telemetry tri-state values in Swift, prefer Optionals (Bool?) over sentinel booleans. In TerminalController.swift, socketConnectable is Bool? and only set when socketProbePerformed is true; downstream logic must treat nil as 'not probed'. Ensure downstream code checks for nil before using a value and uses explicit non-nil checks to determine state, improving clarity and avoiding misinterpretation of default false.
| @AppStorage(UIZoomMetrics.appStorageKey) private var uiZoomScale = UIZoomMetrics.defaultScale | ||
|
|
||
| private let padding: CGFloat = 8 | ||
| private var padding: CGFloat { UIZoomMetrics.searchContainerPadding(uiZoomScale) } | ||
|
|
There was a problem hiding this comment.
Keep barSize in sync with live zoom changes.
The overlay size now depends on uiZoomScale, but barSize is still measured only once in the GeometryReader’s onAppear. If the user changes zoom while the find bar is open, drag snapping keeps using the pre-zoom size until the overlay is recreated.
Suggested fix
.background(
GeometryReader { barGeo in
- Color.clear.onAppear {
- barSize = barGeo.size
- }
+ Color.clear
+ .onAppear {
+ barSize = barGeo.size
+ }
+ .onChange(of: barGeo.size) { _, newSize in
+ barSize = newSize
+ }
}
)Also applies to: 29-38, 101-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Find/BrowserSearchOverlay.swift` around lines 14 - 17, The
GeometryReader currently captures barSize only in its onAppear, so when
uiZoomScale changes the stored barSize (used for drag snapping) becomes stale;
update the code to recompute barSize whenever uiZoomScale changes by reacting to
uiZoomScale (e.g., use onChange(of: uiZoomScale) to recalc and set barSize or
derive barSize from geometry.size and uiZoomScale as a computed property), and
apply the same fix to the other GeometryReader usages that set barSize
(referenced by the barSize variable and the GeometryReader/onAppear blocks).
There was a problem hiding this comment.
Won't fix. barSize is only used in centerPosition() which feeds into closestCorner() for drag-end snapping. The final bar placement is determined by .frame(maxWidth: .infinity, maxHeight: .infinity, alignment: corner.alignment) (line 127) — SwiftUI alignment handles positioning regardless of barSize accuracy. A stale barSize could cause the nearest-corner calculation to pick a slightly wrong corner during a drag, but this requires the user to drag the find bar while simultaneously changing zoom, which is an implausible interaction. The bar cannot overflow or misposition from a stale measurement.
There was a problem hiding this comment.
@moyashin63, that's a fair and well-reasoned call. You're right that the final corner placement is governed by SwiftUI's alignment rather than barSize, making the stale measurement a non-issue for positioning correctness. The only affected code path (closestCorner) is a best-effort heuristic during drag-end, and the simultaneous drag-plus-zoom scenario is contrived enough to not warrant the extra reactive update. Understood — no action needed.
🧠 Learnings used
Learnt from: CR
Repo: manaflow-ai/cmux PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-08T23:03:08.522Z
Learning: Applies to Sources/GhosttyTerminalView.swift : Terminal find layering contract: SurfaceSearchOverlay must be mounted from GhosttySurfaceScrollView in Sources/GhosttyTerminalView.swift (AppKit portal layer), not from SwiftUI panel containers
Learnt from: CR
Repo: manaflow-ai/cmux PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-05T03:20:48.079Z
Learning: Applies to **/*TerminalView.swift : Terminal find layering contract: SurfaceSearchOverlay must be mounted from GhosttySurfaceScrollView in Sources/GhosttyTerminalView.swift (AppKit portal layer), not from SwiftUI panel containers
Learnt from: CR
Repo: manaflow-ai/cmux PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-05T00:05:51.436Z
Learning: Applies to **/GhosttyTerminalView.swift : `SurfaceSearchOverlay` must be mounted from `GhosttySurfaceScrollView` in `Sources/GhosttyTerminalView.swift` (AppKit portal layer), not from SwiftUI panel containers such as `Sources/Panels/TerminalPanelView.swift`
Learnt from: moyashin63
Repo: manaflow-ai/cmux PR: 1074
File: Sources/AppDelegate.swift:7523-7545
Timestamp: 2026-03-09T01:38:24.337Z
Learning: When the command palette is visible (as in manaflow-ai/cmux Sources/AppDelegate.swift), ensure the shortcut handling consumes most Command shortcuts to protect the palette's text input. Specifically, do not allow UI zoom shortcuts (Cmd+Shift+= / Cmd+Shift+− / Cmd+Shift+0) to trigger while the palette is open. Do not reorder shortcut handlers (e.g., uiZoomShortcutAction(...)) to bypass this guard; users must close the palette before performing zoom actions. This guideline should apply to Swift source files handling global shortcuts within the app.
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/AppDelegate.swift:3491-3493
Timestamp: 2026-03-04T14:06:16.241Z
Learning: For manaflow-ai/cmux PR `#819` (Japanese i18n), keep scope limited to localization changes; UX enhancements like preferring workspace.customTitle in workspaceDisplayName() or altering move-target labels should be handled in a separate follow-up issue.
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/ContentView.swift:3194-3196
Timestamp: 2026-03-04T14:05:48.668Z
Learning: In manaflow-ai/cmux (PR `#819`), Sources/ContentView.swift: The command palette’s external window labels intentionally use the global window index from the full orderedSummaries (index + 1), matching the Window menu in AppDelegate. Do not reindex after filtering out the current window to avoid mismatches (“Window 2” for an external window is expected).
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/AppDelegate.swift:0-0
Timestamp: 2026-03-04T14:05:42.574Z
Learning: Guideline: In Swift files (cmux project), when handling pluralized strings, prefer using localization keys with the ICU-style plural forms .one and .other. For example, use keys like statusMenu.unreadCount.one for the singular case (1) and statusMenu.unreadCount.other for all other counts, and similarly for statusMenu.tooltip.unread.one/other. Rationale: ensures correct pluralization across locales and makes localization keys explicit. Review code to ensure any unread count strings and related tooltips follow this .one/.other key pattern and verify the correct value is chosen based on the count.
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 954
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-03-05T22:04:34.712Z
Learning: Adopt the convention: for health/telemetry tri-state values in Swift, prefer Optionals (Bool?) over sentinel booleans. In TerminalController.swift, socketConnectable is Bool? and only set when socketProbePerformed is true; downstream logic must treat nil as 'not probed'. Ensure downstream code checks for nil before using a value and uses explicit non-nil checks to determine state, improving clarity and avoiding misinterpretation of default false.
- Clamp effectiveScale() return value for defensive consistency - Add DEBUG tracing (dlog) for UI zoom shortcut path - Localize "Search" placeholder in BrowserSearchOverlay - Scale notification content (fonts, spacing, corner radii) not just outer padding - Scale address bar button/text fonts and omnibar text field with zoom - Scale suggestion popup text, badges, and paddings with zoom - Scale markdown theme (all heading/code/blockquote sizes and margins) with zoom - Scale fileUnavailableView content with zoom - Remove unused addressBarHPadding, add addressBarSmallFontSize Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/AppDelegate.swift (1)
7523-7545:⚠️ Potential issue | 🟠 MajorThis UI zoom path is still blocked while the command palette is visible.
Line 6999 returns before this block for almost every Command shortcut when the palette is open, so
Cmd+Shift+=/-/0still won't change UI zoom in that state. Move this check above that guard, or exempt these keys insideshouldConsumeShortcutWhileCommandPaletteVisible(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 7523 - 7545, The UI zoom shortcut handling (the uiZoomShortcutAction(...) block that uses UIZoomMetrics and writes to UserDefaults.standard with UIZoomMetrics.appStorageKey) is being skipped when the command palette is visible; move this entire uiZoomShortcutAction handling earlier so it runs before the early return/guard that checks command-palette visibility (or alternatively add an explicit exemption for the zoom keys inside shouldConsumeShortcutWhileCommandPaletteVisible(...) to allow uiZoomShortcutAction to be honored while the palette is shown). Ensure you keep the same behavior for logging (dlog) and the UserDefaults write when moving the block.
🧹 Nitpick comments (4)
Sources/UIZoomMetrics.swift (1)
72-76: Intentionally unscaled minimum sidebar width.The function accepts
scalefor API consistency but returns the base value, which aligns with the PR objective to fix the sidebar drag-resize bug at max zoom. Consider adding a brief inline comment explaining why scale is ignored here for future maintainers.Optional: Add explanatory comment
static let baseMinimumSidebarWidth: Double = 186 + /// Returns the minimum sidebar width. + /// Intentionally ignores scale to prevent drag-resize issues at extreme zoom levels. static func minimumSidebarWidth(_ scale: Double) -> Double { baseMinimumSidebarWidth }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/UIZoomMetrics.swift` around lines 72 - 76, The minimumSidebarWidth(_ scale: Double) function currently ignores its scale parameter and always returns baseMinimumSidebarWidth, which is intentional to fix the sidebar drag-resize bug at max zoom; update the implementation by adding a concise inline comment inside minimumSidebarWidth referencing baseMinimumSidebarWidth and explaining that scale is intentionally ignored for API consistency and to avoid scaling the minimum sidebar width (so future maintainers understand why the parameter is unused).Sources/NotificationsPage.swift (3)
21-21: Consider scaling the LazyVStack spacing.The spacing between notification rows is hardcoded at
8. For full consistency with the UI zoom behavior, this could use a scaled value (e.g.,8 * uiZoomScaleor a dedicatedUIZoomMetricshelper).♻️ Optional fix
- LazyVStack(spacing: 8) { + LazyVStack(spacing: 8 * uiZoomScale) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/NotificationsPage.swift` at line 21, The LazyVStack currently uses a hardcoded spacing of 8 which doesn't respect UI zoom; update the spacing argument in the LazyVStack(...) declaration to use a scaled metric instead (for example replace the literal 8 with 8 * uiZoomScale or a helper like UIZoomMetrics.notificationRowSpacing) so spacing scales with the UI zoom; ensure you reference the same uiZoomScale or UIZoomMetrics value used elsewhere in the view so the LazyVStack spacing stays consistent with other scaled UI elements.
159-173: ShortcutAnnotation scaling is mostly complete.The font size and paddings are correctly scaled. One minor detail: the
cornerRadius: 5on line 170 remains hardcoded while other corner radii in the codebase useUIZoomMetrics.cornerRadius(scale)or similar helpers.♻️ Optional: Scale the corner radius
.background( - RoundedRectangle(cornerRadius: 5) + RoundedRectangle(cornerRadius: UIZoomMetrics.cornerRadius(uiZoomScale) * 0.83) .fill(Color(nsColor: .controlBackgroundColor)) )Alternatively, add a dedicated
shortcutAnnotationCornerRadius(_ scale:)helper toUIZoomMetricsthat returns5 * scale.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/NotificationsPage.swift` around lines 159 - 173, The ShortcutAnnotation view uses a hardcoded cornerRadius(5); change this to use the scaled corner radius helper so the annotation scales with uiZoomScale — either call an existing UIZoomMetrics.cornerRadius(uiZoomScale) or add a new UIZoomMetrics.shortcutAnnotationCornerRadius(_:) (returning 5 * scale) and use that in the RoundedRectangle(cornerRadius: ...) inside ShortcutAnnotation to replace the literal 5.
70-89: Header title font doesn't scale with UIZoomMetrics.The header uses
.font(.title2)which is a semantic size that adapts to system text size settings but won't scale with the app's UI zoom factor. While this may be an intentional design choice for header prominence, it creates a visual inconsistency at extreme zoom levels (0.5× or 2.0×) where everything else scales but the header title remains constant.♻️ Optional: Use scaled font size for consistency
If consistent scaling is desired, replace the semantic font with a scaled size:
HStack { Text(String(localized: "notifications.title", defaultValue: "Notifications")) - .font(.title2) + .font(.system(size: UIZoomMetrics.notificationTitleFontSize(uiZoomScale) * 1.5, weight: .semibold)) .fontWeight(.semibold)Alternatively, add a dedicated
notificationHeaderTitleFontSize(_ scale:)helper toUIZoomMetrics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/NotificationsPage.swift` around lines 70 - 89, The header's title uses a fixed semantic font (.title2) so it doesn't scale with the app's UI zoom; update the header view (variable header) to use a font sized from UIZoomMetrics so it scales with uiZoomScale — e.g., replace the .font(.title2) / .fontWeight(.semibold) usage with a system font constructed from a UIZoomMetrics helper (e.g., notificationHeaderTitleFontSize(uiZoomScale)) and apply the semibold weight there; if needed add notificationHeaderTitleFontSize(_:) to UIZoomMetrics to return the scaled point size and use that in header's font call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/AppDelegate.swift`:
- Around line 7523-7545: The UI zoom shortcut handling (the
uiZoomShortcutAction(...) block that uses UIZoomMetrics and writes to
UserDefaults.standard with UIZoomMetrics.appStorageKey) is being skipped when
the command palette is visible; move this entire uiZoomShortcutAction handling
earlier so it runs before the early return/guard that checks command-palette
visibility (or alternatively add an explicit exemption for the zoom keys inside
shouldConsumeShortcutWhileCommandPaletteVisible(...) to allow
uiZoomShortcutAction to be honored while the palette is shown). Ensure you keep
the same behavior for logging (dlog) and the UserDefaults write when moving the
block.
---
Nitpick comments:
In `@Sources/NotificationsPage.swift`:
- Line 21: The LazyVStack currently uses a hardcoded spacing of 8 which doesn't
respect UI zoom; update the spacing argument in the LazyVStack(...) declaration
to use a scaled metric instead (for example replace the literal 8 with 8 *
uiZoomScale or a helper like UIZoomMetrics.notificationRowSpacing) so spacing
scales with the UI zoom; ensure you reference the same uiZoomScale or
UIZoomMetrics value used elsewhere in the view so the LazyVStack spacing stays
consistent with other scaled UI elements.
- Around line 159-173: The ShortcutAnnotation view uses a hardcoded
cornerRadius(5); change this to use the scaled corner radius helper so the
annotation scales with uiZoomScale — either call an existing
UIZoomMetrics.cornerRadius(uiZoomScale) or add a new
UIZoomMetrics.shortcutAnnotationCornerRadius(_:) (returning 5 * scale) and use
that in the RoundedRectangle(cornerRadius: ...) inside ShortcutAnnotation to
replace the literal 5.
- Around line 70-89: The header's title uses a fixed semantic font (.title2) so
it doesn't scale with the app's UI zoom; update the header view (variable
header) to use a font sized from UIZoomMetrics so it scales with uiZoomScale —
e.g., replace the .font(.title2) / .fontWeight(.semibold) usage with a system
font constructed from a UIZoomMetrics helper (e.g.,
notificationHeaderTitleFontSize(uiZoomScale)) and apply the semibold weight
there; if needed add notificationHeaderTitleFontSize(_:) to UIZoomMetrics to
return the scaled point size and use that in header's font call.
In `@Sources/UIZoomMetrics.swift`:
- Around line 72-76: The minimumSidebarWidth(_ scale: Double) function currently
ignores its scale parameter and always returns baseMinimumSidebarWidth, which is
intentional to fix the sidebar drag-resize bug at max zoom; update the
implementation by adding a concise inline comment inside minimumSidebarWidth
referencing baseMinimumSidebarWidth and explaining that scale is intentionally
ignored for API consistency and to avoid scaling the minimum sidebar width (so
future maintainers understand why the parameter is unused).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bfc64a8a-2ba1-4005-9053-c0880ed35478
📒 Files selected for processing (7)
Resources/Localizable.xcstringsSources/AppDelegate.swiftSources/Find/BrowserSearchOverlay.swiftSources/NotificationsPage.swiftSources/Panels/BrowserPanelView.swiftSources/Panels/MarkdownPanelView.swiftSources/UIZoomMetrics.swift
✅ Files skipped from review due to trivial changes (1)
- Resources/Localizable.xcstrings
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
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="Sources/Panels/BrowserPanelView.swift">
<violation number="1" location="Sources/Panels/BrowserPanelView.swift:3072">
P2: The omnibar text field now scales its font size but is still hard-coded to `.frame(height: 18)`, so text will clip at higher zoom levels (e.g., 2.0x makes the font 24pt). Consider scaling the field height (or adding a metric) alongside the font size.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
The omnibar NSTextField had a hardcoded .frame(height: 18) that did not scale with UI zoom, causing text clipping at scale >= 1.5 where the font size outgrows the fixed frame. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Panels/BrowserPanelView.swift (1)
709-728:⚠️ Potential issue | 🟡 MinorScale the HTTPS lock icon with the rest of the omnibar.
The field font now follows
uiZoomScale, but the secure badge is still hard-coded at10pt. At0.5×it ends up larger than the text, and at2.0×it becomes tiny.Suggested fix
if showSecureBadge { Image(systemName: "lock.fill") - .font(.system(size: 10)) + .font(.system(size: UIZoomMetrics.addressBarSmallFontSize(uiZoomScale))) .foregroundColor(.secondary) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanelView.swift` around lines 709 - 728, The secure badge Image uses a hard-coded font size (system size 10) so it doesn't scale with the omnibar; update the Image modifier where showSecureBadge is rendered (the Image(systemName: "lock.fill") block) to compute its font size from uiZoomScale (e.g., multiply the base badge size by uiZoomScale) so the lock icon scales consistently with the OmnibarTextFieldRepresentable; keep the same modifiers (.foregroundColor(.secondary)) and ensure the computed size is used in the .font(.system(size: ...)) call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Panels/BrowserPanelView.swift`:
- Around line 3238-3245: The fixed 3pt insets (topInset, bottomInset,
horizontalInset) must be converted to scale with uiZoomScale like the other
popup metrics; replace the constants with computed properties that call the
appropriate UIZoomMetrics inset helpers (e.g.,
popupTopInset/popupBottomInset/popupHorizontalInset or a single popupOuterInset
variant) so topInset, bottomInset and horizontalInset derive from
UIZoomMetrics(uiZoomScale), and ensure any calculations that use them
(contentHeight and minimumPopupHeight) continue to reference these updated
properties.
---
Outside diff comments:
In `@Sources/Panels/BrowserPanelView.swift`:
- Around line 709-728: The secure badge Image uses a hard-coded font size
(system size 10) so it doesn't scale with the omnibar; update the Image modifier
where showSecureBadge is rendered (the Image(systemName: "lock.fill") block) to
compute its font size from uiZoomScale (e.g., multiply the base badge size by
uiZoomScale) so the lock icon scales consistently with the
OmnibarTextFieldRepresentable; keep the same modifiers
(.foregroundColor(.secondary)) and ensure the computed size is used in the
.font(.system(size: ...)) call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91a82e73-2350-458e-94db-425648324fd3
📒 Files selected for processing (2)
Sources/Panels/BrowserPanelView.swiftSources/UIZoomMetrics.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/UIZoomMetrics.swift
| private var popupCornerRadius: CGFloat { UIZoomMetrics.popupCornerRadius(uiZoomScale) } | ||
| private var rowHighlightCornerRadius: CGFloat { UIZoomMetrics.popupRowHighlightRadius(uiZoomScale) } | ||
| private var singleLineRowHeight: CGFloat { UIZoomMetrics.popupRowHeight(uiZoomScale) } | ||
| private var rowSpacing: CGFloat { UIZoomMetrics.popupRowSpacing(uiZoomScale) } | ||
| private let topInset: CGFloat = 3 | ||
| private let bottomInset: CGFloat = 3 | ||
| private var horizontalInset: CGFloat { topInset } | ||
| private let maxPopupHeight: CGFloat = 560 | ||
| private var maxPopupHeight: CGFloat { UIZoomMetrics.popupMaxHeight(uiZoomScale) } |
There was a problem hiding this comment.
The popup shell still has fixed outer insets.
rowHeight, rowSpacing, and maxPopupHeight scale, but topInset / bottomInset / horizontalInset stay at 3pt. Because those constants feed contentHeight and minimumPopupHeight, the popup still scales non-linearly at the new zoom extremes.
Suggested fix
- private let topInset: CGFloat = 3
- private let bottomInset: CGFloat = 3
+ private var topInset: CGFloat { UIZoomMetrics.popupOuterInset(uiZoomScale) }
+ private var bottomInset: CGFloat { UIZoomMetrics.popupOuterInset(uiZoomScale) }
private var horizontalInset: CGFloat { topInset }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/BrowserPanelView.swift` around lines 3238 - 3245, The fixed
3pt insets (topInset, bottomInset, horizontalInset) must be converted to scale
with uiZoomScale like the other popup metrics; replace the constants with
computed properties that call the appropriate UIZoomMetrics inset helpers (e.g.,
popupTopInset/popupBottomInset/popupHorizontalInset or a single popupOuterInset
variant) so topInset, bottomInset and horizontalInset derive from
UIZoomMetrics(uiZoomScale), and ensure any calculations that use them
(contentHeight and minimumPopupHeight) continue to reference these updated
properties.
1 participant
Summary
Cmd+Shift+=/-/0) for all non-terminal UI elementsUIZoomMetricsenum with scale-aware static functions for font sizes, paddings, heights, icon sizes across all UI areas@AppStorage("uiZoomScale")Testing
./scripts/reload.sh --tag ui-zoomChecklist
Summary by cubic
Add UI zoom controls (Cmd+Shift+=, Cmd+Shift+-, Cmd+Shift+0) across all non-terminal UI with a 0.5x–2.0x scale persisted in
@AppStorage("uiZoomScale"). Extends scaling to notifications, address bar text, suggestions popup, markdown, and empty/file-unavailable views, localizes the search placeholder, and keeps the terminal font size unchanged.New Features
UIZoomMetricsand applied scaling to titlebar, command palette, browser address bar and suggestions (incl. popup text/badges), search overlays (incl. counter), notifications, markdown panel (headings/code/blockquote sizes and margins), feedback dialog, workspace, and update UI.vendor/bonsplitand passedbonsplitZoomScaleto support tab bar zoom.Bug Fixes
Written for commit a7649b7. Summary will update on new commits.
Summary by CodeRabbit
New Features
Localization
Chores