Repository navigation
Add markdown typography settings - #4106
lawrencecchen wants to merge 4 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThis PR adds user-configurable markdown preview typography and a unified view zoom system. It introduces a typed typography model, settings-file parsing and normalization, UI persistence and theme wiring (CSS variables), schema/docs updates, project build entries, zoom primitives/protocols, panel integrations, key routing, UI controls, and tests. ChangesMarkdown Typography Settings Feature
View Zoom System
Sequence DiagramsequenceDiagram
participant User
participant SettingsFile
participant MarkdownPanelView
participant MarkdownTypographySettings
participant MarkdownWebTheme
participant WebRenderer
participant AppDelegate
participant TabManager
participant Panel as FocusedPanel
User->>SettingsFile: edit `markdown` section
SettingsFile->>MarkdownTypographySettings: parse & normalize
MarkdownTypographySettings->>MarkdownPanelView: read via AppStorage/resolved
MarkdownPanelView->>MarkdownWebTheme: resolve(backgroundColor, typography)
MarkdownWebTheme->>WebRenderer: cssVariables
WebRenderer->>WebRenderer: render with CSS vars
User->>AppDelegate: zoom shortcut
AppDelegate->>TabManager: performViewZoomCommand(command)
TabManager->>Panel: performViewZoomCommand(command) (if conforms)
Panel->>Panel: update viewZoomFactor / apply zoom
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (14 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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Greptile SummaryThis PR adds a
Confidence Score: 5/5Safe to merge — all new settings parsing is per-field validated with invalid values silently ignored, CSS variable injection is adequately guarded, and the ViewZoomControlling refactor is well-contained with unit test coverage for both happy and invalid-input paths. The change is additive and conservative: typography defaults match the existing look, heading sizes now scale with body size rather than being fixed px, and the zoom protocol unification removes ad-hoc browser-only methods. The two observations raised are cosmetic (command-palette stale key for mode-switching FilePreviewPanel) and architectural (zoom interface duplicated across FilePreviewTextEditingPanel), neither affecting correctness in shipped behavior. Sources/Panels/FilePreviewTextEditor.swift — the protocol now carries zoom methods that belong to ViewZoomControlling; worth splitting before more conformers are added. Sources/ContentView.swift — panelSupportsViewZoom is a point-in-time snapshot and will misreport for FilePreviewPanel after a mode switch. Important Files Changed
Sequence DiagramsequenceDiagram
participant CF as cmux.json
participant SFS as CmuxSettingsFileStore
participant UD as UserDefaults
participant MPV as MarkdownPanelView (@AppStorage)
participant MTS as MarkdownTypographySettings
participant MWR as MarkdownWebRenderer (WKWebView)
CF->>SFS: parseMarkdownSection(section)
SFS->>SFS: normalizedFontFamily / normalizedSize
SFS->>UD: "managedUserDefaults[key] = .string/.double"
UD-->>MPV: "@AppStorage observes change (10 keys)"
MPV->>MTS: normalizedFontFamily / normalizedSize (re-validate)
MPV->>MTS: defaultHeadingSizes(forFontSize:) for unset headings
MPV->>MWR: MarkdownWebTheme.resolve(typography:)
MWR->>MWR: applyTheme → CSS variables on WKWebView
note over MPV,MWR: Zoom path
participant TM as TabManager
participant AD as AppDelegate (shortcut monitor)
AD->>TM: performViewZoomCommand(.zoomIn)
TM->>MPV: MarkdownPanel.setViewZoomFactor
MPV->>MWR: zoomFactor binding → WKWebView pageZoom
Reviews (4): Last reviewed commit: "Add View Zoom controls" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| struct MarkdownWebTypography: Equatable { | ||
| struct HeadingSizes: Equatable { | ||
| let h1: Double | ||
| let h2: Double | ||
| let h3: Double | ||
| let h4: Double | ||
| let h5: Double | ||
| let h6: Double | ||
| } | ||
|
|
||
| let fontFamily: String | ||
| let fontSize: Double | ||
| let headingSizes: HeadingSizes | ||
| let codeBlockFontFamily: String | ||
| let codeBlockFontSize: Double | ||
|
|
||
| static let `default` = MarkdownWebTypography( | ||
| fontFamily: MarkdownTypographySettings.defaultFontFamily, | ||
| fontSize: MarkdownTypographySettings.defaultFontSize, | ||
| headingSizes: HeadingSizes( | ||
| h1: MarkdownTypographySettings.defaultHeadingSizes.h1, | ||
| h2: MarkdownTypographySettings.defaultHeadingSizes.h2, | ||
| h3: MarkdownTypographySettings.defaultHeadingSizes.h3, | ||
| h4: MarkdownTypographySettings.defaultHeadingSizes.h4, | ||
| h5: MarkdownTypographySettings.defaultHeadingSizes.h5, | ||
| h6: MarkdownTypographySettings.defaultHeadingSizes.h6 | ||
| ), | ||
| codeBlockFontFamily: MarkdownTypographySettings.defaultCodeBlockFontFamily, | ||
| codeBlockFontSize: MarkdownTypographySettings.defaultCodeBlockFontSize | ||
| ) | ||
|
|
||
| var cssVariables: [String: String] { | ||
| [ | ||
| "--cmux-markdown-font-family": fontFamily, | ||
| "--cmux-markdown-font-size": Self.cssPixels(fontSize), | ||
| "--cmux-markdown-h1-font-size": Self.cssPixels(headingSizes.h1), | ||
| "--cmux-markdown-h2-font-size": Self.cssPixels(headingSizes.h2), | ||
| "--cmux-markdown-h3-font-size": Self.cssPixels(headingSizes.h3), | ||
| "--cmux-markdown-h4-font-size": Self.cssPixels(headingSizes.h4), | ||
| "--cmux-markdown-h5-font-size": Self.cssPixels(headingSizes.h5), | ||
| "--cmux-markdown-h6-font-size": Self.cssPixels(headingSizes.h6), | ||
| "--cmux-markdown-code-block-font-family": codeBlockFontFamily, | ||
| "--cmux-markdown-code-block-font-size": Self.cssPixels(codeBlockFontSize), | ||
| ] | ||
| } | ||
|
|
||
| private static func cssPixels(_ value: Double) -> String { | ||
| let rounded = (value * 100).rounded() / 100 | ||
| if rounded.rounded() == rounded { | ||
| return "\(Int(rounded))px" | ||
| } | ||
| return "\(rounded)px" | ||
| } | ||
| } | ||
|
|
||
| enum MarkdownTypographySettings { | ||
| struct HeadingSizes: Equatable { | ||
| let h1: Double | ||
| let h2: Double | ||
| let h3: Double | ||
| let h4: Double | ||
| let h5: Double | ||
| let h6: Double | ||
| } |
There was a problem hiding this comment.
nonisolated on pure value types
MarkdownWebTypography and MarkdownTypographySettings are pure value/static-method types with no SwiftUI or AppKit lifecycle dependency. In Swift 6's MainActor-by-default isolation mode (active in this module), both types will be implicitly @MainActor, meaning callers on any other actor must await even the pure normalizedFontFamily, normalizedSize, and resolved(defaults:) helpers unnecessarily. Both types should be prefixed with nonisolated, matching the preferred shape nonisolated struct Model: Sendable { … } and nonisolated enum Namespace { … }. MarkdownTypographySettings.resolved(defaults:) uses only UserDefaults (thread-safe) and pure math — there is no reason to require main-actor access.
Rule Used: Flag new or materially worsened Swift 6 actor isol... (source)
| static let defaultHeadingSizes = HeadingSizes( | ||
| h1: 30, | ||
| h2: 22.5, | ||
| h3: 18.75, | ||
| h4: 15, | ||
| h5: 13.13, | ||
| h6: 12.75 | ||
| ) |
There was a problem hiding this comment.
Heading sizes are stored and applied as fixed px values. When a user sets markdown.fontSize: 20, the heading defaults stay at 30, 22.5, 18.75, 15, 13.13, and 12.75 px — meaning h4 through h6 are literally smaller than the body text. The CSS fallbacks (2em, 1.5em, etc.) in shell.html scale correctly, but once applyTheme fires those are replaced by the fixed-px CSS variables. Worth documenting this in the schema description or auto-deriving default heading sizes relative to the configured body size. Was the intent for heading sizes to be absolute px only, or should they have a relative/em option? Changing body font without touching headings currently produces headings smaller than body text for h4-h6.
| struct MarkdownWebTypography: Equatable { | ||
| struct HeadingSizes: Equatable { | ||
| let h1: Double | ||
| let h2: Double | ||
| let h3: Double | ||
| let h4: Double | ||
| let h5: Double | ||
| let h6: Double | ||
| } |
There was a problem hiding this comment.
HeadingSizes struct in the same file
MarkdownWebTypography.HeadingSizes and MarkdownTypographySettings.HeadingSizes have identical fields (h1–h6: Double). The settings type stores defaults as MarkdownTypographySettings.HeadingSizes then copies each field one-by-one into MarkdownWebTypography.HeadingSizes at every call site. Adding a new heading level would require updating both struct definitions and every construction site. MarkdownTypographySettings.HeadingSizes can be removed and the defaults changed to use MarkdownWebTypography.HeadingSizes directly.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 86bf9be. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/MarkdownPanelTests.swift`:
- Around line 67-70: The test currently asserts only h1 and h3 CSS variables but
the setup contains headingSizes.h1...h6, so add assertions for the remaining
heading keys to fully validate the mapping; specifically, in
MarkdownPanelTests.swift extend the XCTAssertEqual checks on theme.cssVariables
to include "--cmux-markdown-h2-font-size", "--cmux-markdown-h4-font-size",
"--cmux-markdown-h5-font-size", and "--cmux-markdown-h6-font-size" using the
expected values from headingSizes.h2/h4/h5/h6 so the test verifies all h1–h6
mappings alongside the existing code-block assertions.
🪄 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: 8e847dad-be88-4c6b-8b63-4dd05de3e786
📒 Files selected for processing (11)
GhosttyTabs.xcodeproj/project.pbxprojResources/markdown-viewer/shell.htmlSources/KeyboardShortcutSettingsFileStore+Template.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/Panels/MarkdownPanelView.swiftSources/Panels/MarkdownTypographySettings.swiftSources/Panels/MarkdownWebRenderer.swiftcmuxTests/MarkdownPanelTests.swiftcmuxTests/WorkspaceUnitTests.swiftweb/app/[locale]/docs/configuration/page.tsxweb/data/cmux.schema.json
| private var markdownTypography: MarkdownWebTypography { | ||
| _ = markdownFontFamily | ||
| _ = markdownFontSize | ||
| _ = markdownHeadingH1Size | ||
| _ = markdownHeadingH2Size | ||
| _ = markdownHeadingH3Size | ||
| _ = markdownHeadingH4Size | ||
| _ = markdownHeadingH5Size | ||
| _ = markdownHeadingH6Size | ||
| _ = markdownCodeBlockFontFamily | ||
| _ = markdownCodeBlockFontSize | ||
| return MarkdownTypographySettings.resolved() | ||
| } |
There was a problem hiding this comment.
@AppStorage and resolved()
All 10 @AppStorage properties are read only to be discarded (_ = markdownFontFamily etc.); the theme is computed entirely from a separate, independent UserDefaults.standard read via MarkdownTypographySettings.resolved(). The @AppStorage bindings act as change-notification side-channels while the actual data flows through resolved().
Today this works because both use .standard as the backing store. But if a managed-settings UserDefaults suite is ever introduced (a natural next step given the existing managedUserDefaults path in CmuxSettingsFileStore), the @AppStorage bindings would observe the managed suite while resolved() still reads .standard — the view would stop updating. The fix is to compute MarkdownWebTypography directly from the @AppStorage values instead of discarding them and re-reading from UserDefaults.
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/Panels/MarkdownPanelView.swift`:
- Around line 211-223: Add a brief comment above the computed property
markdownTypography explaining the `@AppStorage` dependency pattern: that the
property intentionally reads each `@AppStorage-backed` value (markdownFontFamily,
markdownFontSize, markdownHeadingH1Size, markdownHeadingH2Size,
markdownHeadingH3Size, markdownHeadingH4Size, markdownHeadingH5Size,
markdownHeadingH6Size, markdownCodeBlockFontFamily, markdownCodeBlockFontSize)
solely to establish SwiftUI change-tracking so the view updates, and that
MarkdownTypographySettings.resolved() is used to centralize
validation/normalization from UserDefaults; keep the comment short and focused
so future maintainers understand why the values are read but not directly used.
🪄 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: 44f99610-1989-4d93-92dd-9209eb344faf
📒 Files selected for processing (6)
Resources/markdown-viewer/shell.htmlSources/Panels/MarkdownPanelView.swiftSources/Panels/MarkdownTypographySettings.swiftcmuxTests/MarkdownPanelTests.swiftcmuxTests/WorkspaceUnitTests.swiftweb/data/cmux.schema.json
| private var markdownTypography: MarkdownWebTypography { | ||
| _ = markdownFontFamily | ||
| _ = markdownFontSize | ||
| _ = markdownHeadingH1Size | ||
| _ = markdownHeadingH2Size | ||
| _ = markdownHeadingH3Size | ||
| _ = markdownHeadingH4Size | ||
| _ = markdownHeadingH5Size | ||
| _ = markdownHeadingH6Size | ||
| _ = markdownCodeBlockFontFamily | ||
| _ = markdownCodeBlockFontSize | ||
| return MarkdownTypographySettings.resolved() | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider a brief comment explaining the @AppStorage dependency pattern.
The computed property references all @AppStorage properties (lines 212–221) to establish SwiftUI dependencies, then calls resolved() to read and normalize the values from UserDefaults. This pattern keeps validation logic centralized in MarkdownTypographySettings and ensures the view updates when any typography setting changes.
A brief comment would help future maintainers understand why the properties are read but not directly used.
📝 Suggested comment
private var markdownTypography: MarkdownWebTypography {
+ // Reference each `@AppStorage` property to establish SwiftUI dependencies;
+ // resolved() reads and normalizes the values from UserDefaults.
_ = markdownFontFamily🤖 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/Panels/MarkdownPanelView.swift` around lines 211 - 223, Add a brief
comment above the computed property markdownTypography explaining the
`@AppStorage` dependency pattern: that the property intentionally reads each
`@AppStorage-backed` value (markdownFontFamily, markdownFontSize,
markdownHeadingH1Size, markdownHeadingH2Size, markdownHeadingH3Size,
markdownHeadingH4Size, markdownHeadingH5Size, markdownHeadingH6Size,
markdownCodeBlockFontFamily, markdownCodeBlockFontSize) solely to establish
SwiftUI change-tracking so the view updates, and that
MarkdownTypographySettings.resolved() is used to centralize
validation/normalization from UserDefaults; keep the comment short and focused
so future maintainers understand why the values are read but not directly used.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/Panels/MarkdownPanelView.swift (1)
211-261: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winEliminate duplicated validation logic by delegating to
MarkdownTypographySettings.resolved().The computed property reimplements all the validation logic (normalizing font families, validating sizes, deriving heading sizes) that already exists in
MarkdownTypographySettings.resolved(). This violates DRY and creates maintenance risk—the two implementations can drift.Refactor to reference each
@AppStorageproperty (establishing SwiftUI dependencies), then delegate to the centralized resolution logic.♻️ Proposed refactor
private var markdownTypography: MarkdownWebTypography { - let bodyFontSize = MarkdownTypographySettings.normalizedSize( - markdownFontSize, - range: MarkdownTypographySettings.fontSizeRange - ) ?? MarkdownTypographySettings.defaultFontSize - let derivedHeadingSizes = MarkdownTypographySettings.defaultHeadingSizes(forFontSize: bodyFontSize) - return MarkdownWebTypography( - fontFamily: MarkdownTypographySettings.normalizedFontFamily(markdownFontFamily) - ?? MarkdownTypographySettings.defaultFontFamily, - fontSize: bodyFontSize, - headingSizes: MarkdownWebTypography.HeadingSizes( - h1: markdownHeadingSize( - markdownHeadingH1Size, - fallback: derivedHeadingSizes.h1 - ), - h2: markdownHeadingSize( - markdownHeadingH2Size, - fallback: derivedHeadingSizes.h2 - ), - h3: markdownHeadingSize( - markdownHeadingH3Size, - fallback: derivedHeadingSizes.h3 - ), - h4: markdownHeadingSize( - markdownHeadingH4Size, - fallback: derivedHeadingSizes.h4 - ), - h5: markdownHeadingSize( - markdownHeadingH5Size, - fallback: derivedHeadingSizes.h5 - ), - h6: markdownHeadingSize( - markdownHeadingH6Size, - fallback: derivedHeadingSizes.h6 - ) - ), - codeBlockFontFamily: MarkdownTypographySettings.normalizedFontFamily(markdownCodeBlockFontFamily) - ?? MarkdownTypographySettings.defaultCodeBlockFontFamily, - codeBlockFontSize: MarkdownTypographySettings.normalizedSize( - markdownCodeBlockFontSize, - range: MarkdownTypographySettings.codeBlockFontSizeRange - ) ?? MarkdownTypographySettings.defaultCodeBlockFontSize - ) - } - - private func markdownHeadingSize(_ raw: Double, fallback: Double) -> Double { - return MarkdownTypographySettings.normalizedSize( - raw, - range: MarkdownTypographySettings.headingSizeRange - ) ?? fallback + // Reference each `@AppStorage` property to establish SwiftUI dependencies. + _ = markdownFontFamily + _ = markdownFontSize + _ = markdownHeadingH1Size + _ = markdownHeadingH2Size + _ = markdownHeadingH3Size + _ = markdownHeadingH4Size + _ = markdownHeadingH5Size + _ = markdownHeadingH6Size + _ = markdownCodeBlockFontFamily + _ = markdownCodeBlockFontSize + // Delegate to centralized validation/resolution logic. + return MarkdownTypographySettings.resolved() }🤖 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/Panels/MarkdownPanelView.swift` around lines 211 - 261, The markdownTypography computed property duplicates validation/normalization logic; replace its body to simply construct and return the resolved typography by delegating to MarkdownTypographySettings.resolved(...) (using the local `@AppStorage-backed` properties such as markdownFontSize, markdownFontFamily, markdownHeadingH1Size... and markdownCodeBlockFontFamily/markdownCodeBlockFontSize) and remove/stop using the local markdownHeadingSize helper for heading normalization so all validation is centralized in MarkdownTypographySettings.resolved().
🤖 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.
Duplicate comments:
In `@Sources/Panels/MarkdownPanelView.swift`:
- Around line 211-261: The markdownTypography computed property duplicates
validation/normalization logic; replace its body to simply construct and return
the resolved typography by delegating to
MarkdownTypographySettings.resolved(...) (using the local `@AppStorage-backed`
properties such as markdownFontSize, markdownFontFamily,
markdownHeadingH1Size... and
markdownCodeBlockFontFamily/markdownCodeBlockFontSize) and remove/stop using the
local markdownHeadingSize helper for heading normalization so all validation is
centralized in MarkdownTypographySettings.resolved().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4aa2c849-b24f-4a8f-b86c-a150d4f66fc6
📒 Files selected for processing (2)
Sources/Panels/MarkdownPanelView.swiftSources/Panels/MarkdownTypographySettings.swift
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/AppDelegate.swift`:
- Around line 12106-12118: The focused-terminal zoom handoff currently only
checks browserZoomShortcutAction(...) which misses user-remapped zoom shortcuts;
update shouldLetFocusedTerminalHandleViewZoomShortcut(_:) to also consult
configuredViewZoomShortcutCommand(for:) (using the same event data:
modifierFlags, charactersIgnoringModifiers, keyCode, literal chars) before
returning false, so if configuredViewZoomShortcutCommand(...) resolves to a zoom
command the method returns true and lets the terminal (cmuxOwningGhosttyView for
firstResponder / resolvedShortcutEventWindow) handle the shortcut.
🪄 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: 1d07bf5f-0cc9-48a4-8529-ab5bd6f43a65
📒 Files selected for processing (19)
GhosttyTabs.xcodeproj/project.pbxprojResources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/KeyboardShortcutContext.swiftSources/Panels/BrowserPanel.swiftSources/Panels/FilePreviewMagnifyingPDFView.swiftSources/Panels/FilePreviewPanel.swiftSources/Panels/FilePreviewTextEditor.swiftSources/Panels/MarkdownPanel.swiftSources/Panels/MarkdownPanelView.swiftSources/Panels/MarkdownWebRenderer.swiftSources/Panels/PanelContentView.swiftSources/Panels/ViewZoomControls.swiftSources/TabManager.swiftSources/cmuxApp.swiftcmuxTests/BrowserConfigTests.swiftcmuxTests/KeyboardShortcutContextTests.swiftcmuxTests/WindowAndDragTests.swift
| private func shouldLetFocusedTerminalHandleViewZoomShortcut(_ event: NSEvent) -> Bool { | ||
| let targetWindow = resolvedShortcutEventWindow(event) ?? event.window ?? NSApp.keyWindow ?? NSApp.mainWindow | ||
| guard let firstResponder = targetWindow?.firstResponder, | ||
| cmuxOwningGhosttyView(for: firstResponder) != nil else { | ||
| return false | ||
| } | ||
| return browserZoomShortcutAction( | ||
| flags: event.modifierFlags, | ||
| chars: event.charactersIgnoringModifiers ?? "", | ||
| keyCode: event.keyCode, | ||
| literalChars: event.characters | ||
| ) != nil | ||
| } |
There was a problem hiding this comment.
Terminal handoff check misses remapped zoom shortcuts
shouldLetFocusedTerminalHandleViewZoomShortcut only returns true when browserZoomShortcutAction(...) matches the literal key pattern. That bypasses custom-configured zoom bindings (which do resolve in configuredViewZoomShortcutCommand(for:)), so terminal-focused remapped zoom shortcuts can be incorrectly consumed by AppDelegate.
🔧 Suggested fix
private func shouldLetFocusedTerminalHandleViewZoomShortcut(_ event: NSEvent) -> Bool {
let targetWindow = resolvedShortcutEventWindow(event) ?? event.window ?? NSApp.keyWindow ?? NSApp.mainWindow
guard let firstResponder = targetWindow?.firstResponder,
cmuxOwningGhosttyView(for: firstResponder) != nil else {
return false
}
- return browserZoomShortcutAction(
- flags: event.modifierFlags,
- chars: event.charactersIgnoringModifiers ?? "",
- keyCode: event.keyCode,
- literalChars: event.characters
- ) != nil
+ return true
}🤖 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/AppDelegate.swift` around lines 12106 - 12118, The focused-terminal
zoom handoff currently only checks browserZoomShortcutAction(...) which misses
user-remapped zoom shortcuts; update
shouldLetFocusedTerminalHandleViewZoomShortcut(_:) to also consult
configuredViewZoomShortcutCommand(for:) (using the same event data:
modifierFlags, charactersIgnoringModifiers, keyCode, literal chars) before
returning false, so if configuredViewZoomShortcutCommand(...) resolves to a zoom
command the method returns true and lets the terminal (cmuxOwningGhosttyView for
firstResponder / resolvedShortcutEventWindow) handle the shortcut.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
Acceptance criteria
Verification
Demo
Note
Medium Risk
Moderate risk because it adds new settings parsing/validation and pushes additional CSS variables into the Markdown WebView theme, which could affect markdown rendering and live updates if misapplied.
Overview
Adds a new
markdownsection tocmux.jsonto configure Markdown preview typography (body/heading/code-block font families and sizes), with per-field validation so invalid values are ignored while valid siblings still apply.Threads the resolved typography from
UserDefaults/@AppStoragethroughMarkdownPanelViewintoMarkdownWebTheme, and updates the markdown viewershell.htmlto consume the new CSS variables for font stacks and heading/code sizes.Updates the config template, JSON schema, docs, and unit tests to cover CSS-variable output and settings-file application/invalid-value handling.
Reviewed by Cursor Bugbot for commit f34bd19. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds configurable Markdown typography in
cmux.jsonand introduces unified View Zoom controls across Markdown, Browser, and text previews. Settings apply live via CSS variables and WebView zoom; zoom is accessible by shortcuts, menu, command palette, and a new header popover.New Features
fontFamily,fontSize,headingSizes.h1–h6,codeBlockFontFamily,codeBlockFontSize; unset headings derive fromfontSize; frontmatter uses code font/size.@AppStorage→ Markdown theme CSS variables andshell.htmlfont/size variables; docs,cmux.schema.json, template updated; tests cover parsing and CSS output.browserZoomIn/Out/Resetacross Browser, Markdown, and text preview; per-panel zoom factor persists and updatesWKWebView.pageZoomand text editor font size; command palette entries appear when supported; new header “View Zoom” popover with +/- buttons, reset, and slider; PDF/image viewers handle zoom key equivalents.Bug Fixes
@AppStoragetypography so open panels update without reload.Written for commit 314a3c9. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Tests
Localization