Add markdown viewer font size controls - #5153
lawrencecchen wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR adds configurable font sizing for markdown viewer panels, including a global ChangesMarkdown Viewer Font Size Control
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (4 errors, 1 warning, 1 inconclusive)
✅ Passed checks (12 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5445ef5170
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if matchConfiguredShortcut(event: event, action: .markdownZoomIn) { | ||
| return shortcutEventMarkdownPanel(event)?.zoomIn() ?? false |
There was a problem hiding this comment.
This only handles markdown zoom in the low-level shortcut path, while the existing View menu and command-palette zoom entries still route the same ⌘=/⌘-/⌘0 commands to browser-only actions (Sources/cmuxApp.swift:847 and Sources/ContentView.swift:8168). When a MarkdownPanel is focused, the keyboard shortcut changes markdown size, but choosing View > Zoom In/Out/Actual Size or the palette command no-ops/beeps because there is no focused browser. Please route zoom through one focused-panel action or add markdown-aware menu/palette entries so the advertised controls behave consistently.
Useful? React with 👍 / 👎.
5445ef5 to
bde31ef
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR adds configurable font size controls to the cmux markdown viewer: a new
Confidence Score: 5/5Safe to merge — changes are self-contained to the markdown viewer surface with no mutations to shared state outside of a clearly isolated per-panel override model. Actor isolation is correctly established (MarkdownPanel is @mainactor), the Combine subscription uses a stored AnyCancellable and properly cancels on close, CSS variable injection is value-safe, and font-size normalization is applied consistently across all code paths (init, CLI, settings UI, session restore). The keyboard shortcut context correctly marks .markdownPanel as overlapping .nonBrowserPanel so conflict detection fires, and the AppDelegate shortcut handler processes markdown zoom before browser zoom so events are consumed correctly when a markdown panel is focused. No files require special attention — the one style note in AppSection.swift (a redundant MainActor.run inside an already-MainActor Task) has no behavioral impact. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant AppDelegate
participant MarkdownPanel
participant MarkdownWebRenderer
participant WKWebView
Note over User,WKWebView: Keyboard shortcut zoom (⌘=)
User->>AppDelegate: "NSEvent (Cmd+=)"
AppDelegate->>AppDelegate: matchConfiguredShortcut(.markdownZoomIn)
AppDelegate->>MarkdownPanel: zoomIn()
MarkdownPanel->>MarkdownPanel: "fontSizeOverridePoints = normalized"
MarkdownPanel->>MarkdownPanel: "fontSizePoints = normalized (@Published)"
MarkdownPanel-->>AppDelegate: true (event consumed)
MarkdownPanel->>MarkdownWebRenderer: updateNSView (fontSizePoints changed)
MarkdownWebRenderer->>WKWebView: applyFontSize() via JS CSS vars
Note over User,WKWebView: Settings UI change
User->>AppSection: Stepper increment
AppSection->>UserDefaults: defaultsStore.set(clamped)
AppSection->>NotificationCenter: post(markdownViewerFontSizeDidChange)
NotificationCenter->>MarkdownPanel: sink (if no override)
MarkdownPanel->>MarkdownPanel: "fontSizePoints = resolved() (@Published)"
MarkdownPanel->>MarkdownWebRenderer: updateNSView → applyFontSize()
Note over User,WKWebView: resetZoom (⌘0)
User->>AppDelegate: NSEvent (Cmd+0)
AppDelegate->>MarkdownPanel: resetZoom()
MarkdownPanel->>MarkdownPanel: "fontSizeOverridePoints = nil"
MarkdownPanel->>MarkdownPanel: "fontSizePoints = resolved() (@Published)"
MarkdownPanel->>MarkdownWebRenderer: updateNSView → applyFontSize()
Reviews (2): Last reviewed commit: "Add markdown viewer font size controls" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "shortcut.markdownZoomIn.label": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Markdown Viewer: Zoom In" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Markdown ビューア: 拡大" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "shortcut.markdownZoomOut.label": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Markdown Viewer: Zoom Out" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Markdown ビューア: 縮小" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "shortcut.markdownZoomReset.label": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Markdown Viewer: Actual Size" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Markdown ビューア: 実際のサイズ" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "shortcut.saveFilePreview.label": { |
There was a problem hiding this comment.
Every new user-facing key added by this PR (shortcut.markdownZoomIn.label, shortcut.markdownZoomOut.label, shortcut.markdownZoomReset.label, settings.app.markdownViewerFontSize, settings.app.markdownViewerFontSize.subtitle, and the search alias) only has en and ja entries. The existing catalog already carries translated entries for 20 locales — ar, bs, da, de, es, fr, it, km, ko, nb, pl, pt-BR, ru, th, tr, uk, zh-Hans, zh-Hant plus en and ja. Comparable keys like command.browserZoomIn.title (the closest existing analog) have all 19 non-km translations. Users in every non-Japanese, non-English locale will see raw English strings in the Settings keyboard-shortcuts panel and the Settings search index for all six new keys.
Rule Used: Flag production user-facing text that is not fully... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| userDefaultsCancellable = NotificationCenter.default | ||
| .publisher(for: UserDefaults.didChangeNotification) | ||
| .receive(on: DispatchQueue.main) | ||
| .sink { [weak self] _ in | ||
| Task { @MainActor [weak self] in | ||
| self?.refreshDefaultFontSizeIfNeeded() | ||
| } | ||
| } |
There was a problem hiding this comment.
UserDefaults.didChangeNotification fires on every key, not just markdownViewerFontSize
UserDefaults.didChangeNotification is posted for any UserDefaults write in the process — shortcut changes, terminal color updates, session writes, and so on. Every open MarkdownPanel now registers a live subscriber to this notification, so N panels × M unrelated writes = N×M calls to refreshDefaultFontSizeIfNeeded() per session. The guard on fontSizeOverridePoints and the equality check in updateFontSize keep the overhead small per call, but this is a hot-path subscription attached to an unbounded fanout. Additionally, when a cmux.json sync fires MarkdownViewerFontSizeSettings.didChangeNotification, the UserDefaults.didChangeNotification fires immediately afterwards too, so refreshDefaultFontSizeIfNeeded() is called twice per panel. Prefer UserDefaults.standard.observe(\.markdownViewerFontSize, options: .new) with a typed KVO observer scoped to the specific key, or consolidate on the existing custom notification and drop userDefaultsCancellable.
Rule Used: Flag SwiftUI changes that can cause stale state, b... (source)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Packages/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction.swift`:
- Line 121: The hard-coded English titles for shortcut metadata (e.g., the
.markdown case returning "Markdown Viewer" in ShortcutAction.swift and the
similar strings at the other noted cases) must be replaced with localized
strings and corresponding catalog entries; update the Group.title / displayName
providers in ShortcutAction (and the other occurrences referenced at 231-233) to
use a localization API (e.g., NSLocalizedString or String(localized:)) with
unique keys like "ShortcutAction.Markdown.title" /
"ShortcutAction.Markdown.displayName", then add matching entries to the app's
Localizable.strings (or the SwiftGen/localizable catalog) so
KeyboardShortcutsSection rendering action.displayName shows translated text.
Ensure keys are descriptive and consistent across all cases.
In
`@Packages/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/CuratedSettingEntry`+Default.swift:
- Line 44: The curated entry created via CuratedSettingEntry.init(section: .app,
id: "markdown-font-size", title: "Markdown Viewer Font Size", synonyms:
"app.markdownViewerFontSize markdown viewer font size text scale zoom points
preview") must not hard-code English; replace the literal title and synonyms
with localized lookups (e.g., NSLocalizedString or localized string key API) and
add corresponding keys/values to the app's Localizable.strings catalog for all
supported locales. Update the CuratedSettingEntry creation to reference the
localized title key and a localized synonyms string (or localized array) and
ensure matching entries exist in the strings catalog so both the visible title
and search terms are localized consistently.
In `@Resources/markdown-viewer/shell.html`:
- Around line 16-21: The frontmatter summary is hard-coded to 13px and doesn't
respect the zoom variable; update the CSS rules for .cmux-frontmatter summary
(and the other occurrences noted) to replace the fixed "13px" with the scaling
variable, e.g. use var(--cmux-markdown-font-size) (or a small calc() like
calc(var(--cmux-markdown-font-size) * 0.87) if you want it slightly smaller) so
the frontmatter affordance scales with the panel; apply the same change to the
other referenced selectors at lines ~39-40 and ~210-216.
In `@Sources/App/WorkspaceRuntimeSettings.swift`:
- Around line 99-105: The validation currently allows any positive finite points
but minimumPoints is 1, causing stored values like 0.5 to be considered usable
even though resolved() clamps to 1; update isUsable(_:) to require
points.isFinite && points >= minimumPoints && points <= maximumPoints so stored
values respect the documented 1...96 range, and apply the same >= minimumPoints
change to the analogous validation/normalization check later in the file (the
block around lines 111-114 that mirrors isUsable(_:)); ensure references to
minimumPoints, maximumPoints, zoomStepPoints, isUsable(_:) and resolved() are
used so the enforced contract is consistent between storage and runtime
resolution.
In `@Sources/KeyboardShortcutContext.swift`:
- Around line 27-40: The .markdownPanel availability currently ignores
rightSidebarFocused so markdownZoomIn/Out/Reset (mapped to .markdownPanel) can
fire when the right sidebar is focused; update
KeyboardShortcutContext.isAvailable so the .markdownPanel branch also checks
that rightSidebarFocused is false (e.g., return focusedMarkdownPanel &&
!rightSidebarFocused) to suppress markdown zoom while the right sidebar is
focused; ensure you only change the .markdownPanel case and leave other cases
(.application, .nonBrowserPanel, .browserPanel, .rightSidebarFocus) unchanged.
In `@web/messages/es.json`:
- Line 635: The "markdown" translation key in es.json currently contains the
English string "Markdown Viewer"; replace its value with a Spanish translation
(e.g., "Visor de Markdown") so the key "markdown" in the Spanish locale is fully
localized and consistent with other high-confidence locales.
In `@web/messages/fr.json`:
- Line 635: The French locale file has the "markdown" key still in English;
update the value for the "markdown" message key in fr.json to the French
translation (e.g., "Visionneuse Markdown" or equivalent approved translation) so
the "markdown" entry is localized consistently with other high-confidence
entries; locate the "markdown" key in web/messages/fr.json and replace "Markdown
Viewer" with the correct French string.
In `@web/messages/it.json`:
- Line 635: The Italian locale entry for the "markdown" category key currently
uses the English value "Markdown Viewer"; update the value for the "markdown"
key in the it.json locale to an Italian translation (e.g., "Visualizzatore
Markdown" or "Visualizzatore di Markdown") so the category label is localized
and consistent with other Italian UI copy.
In `@web/messages/ko.json`:
- Line 635: Update the Korean translation for the keyboard shortcuts category
key keyboardShortcuts.cat.markdown in web/messages/ko.json by replacing the
current English value "Markdown Viewer" with the Korean copy "마크다운 뷰어" so the
shortcuts UI is fully localized for ko.
In `@web/messages/no.json`:
- Line 635: The "markdown" category label is still English; update the Norwegian
locale entry for the key "markdown" in the provided JSON (the line with
"markdown": "Markdown Viewer") to a Norwegian translation (e.g.,
"Markdown-visning" or similar) so the category list is consistently localized
for the 'no' locale.
In `@web/messages/pt-BR.json`:
- Line 635: The "markdown" category label currently reads "Markdown Viewer" in
the pt-BR locale; update the JSON value for the "markdown" key in
web/messages/pt-BR.json to a Portuguese translation consistent with other labels
(e.g., "Visualizador de Markdown" or "Visualizador Markdown") so the category
matches the localized style used for keys like "diffViewer".
In `@web/messages/ru.json`:
- Line 635: Replace the English label for docs.keyboardShortcuts.cat.markdown in
the Russian locale with the translated Russian text (e.g., "Просмотрщик
Markdown") so the key docs.keyboardShortcuts.cat.markdown is fully localized in
the ru locale file; update the value where "markdown": "Markdown Viewer" (or the
equivalent docs.keyboardShortcuts.cat.markdown entry) appears to the Russian
string.
In `@web/messages/tr.json`:
- Line 635: Update the Turkish translation for the keyboard shortcuts category
key docs.keyboardShortcuts.cat.markdown from "Markdown Viewer" to a Turkish
label (suggested "Markdown Görüntüleyici" or equivalent) so the locale contains
a translated user-facing category string; locate the
docs.keyboardShortcuts.cat.markdown entry and replace the English value with the
Turkish translation.
In `@web/messages/uk.json`:
- Line 636: The "markdown" message key in the Ukrainian locale file (uk.json) is
still English ("Markdown Viewer"); replace its value with the correct Ukrainian
translation for the shortcut category label under the "markdown" key, making
sure the string is properly escaped/quoted and preserves any punctuation; also
update the corresponding message entries in every other locale listed in
web/i18n/routing.ts so all locales remain in sync with this new shortcut
category.
In `@web/messages/zh-CN.json`:
- Line 635: The zh-CN locale file currently contains an English fallback for the
"markdown" key ("Markdown Viewer"); replace that value with a Simplified Chinese
translation (e.g., "Markdown 查看器" or "Markdown 预览器") so the "markdown" key in
web/messages/zh-CN.json is localized for zh-CN; ensure the JSON string value
remains properly quoted and escaped.
In `@web/messages/zh-TW.json`:
- Line 635: The zh-TW locale currently has the English string for the "markdown"
key; replace the value of the "markdown" entry with a Traditional Chinese
translation (e.g. "Markdown 檢視器") so the "markdown" key in
web/messages/zh-TW.json contains localized Traditional Chinese text rather than
English.
🪄 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: 0b741ea3-efd8-42eb-acbd-ac282738cd03
📒 Files selected for processing (56)
CLI/cmux.swiftPackages/CmuxSettings/Sources/CmuxSettings/Keys/AppCatalogSection.swiftPackages/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction+Defaults.swiftPackages/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction.swiftPackages/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/CuratedSettingEntry+Default.swiftPackages/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/AppSection.swiftPackages/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/KeyboardShortcutsSection.swiftPackages/CmuxSettingsUI/Tests/CmuxSettingsUITests/SettingsRowAnchorResolutionTests.swiftREADME.mdResources/Localizable.xcstringsResources/markdown-viewer/shell.htmlSources/App/WorkspaceRuntimeSettings.swiftSources/AppDelegate.swiftSources/CmuxSettingsJSONPathSupport.swiftSources/ContentView.swiftSources/KeyboardShortcutContext.swiftSources/KeyboardShortcutSettings.swiftSources/KeyboardShortcutSettingsFileStore+Template.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/Panels/MarkdownPanel.swiftSources/Panels/MarkdownPanelView.swiftSources/Panels/MarkdownWebRenderer.swiftSources/SessionPersistence.swiftSources/SettingsNavigation.swiftSources/SettingsSearchAliases.swiftSources/TerminalController.swiftSources/Workspace.swiftcmuxTests/KeyboardShortcutContextTests.swiftcmuxTests/KeyboardShortcutSettingsFileStoreStartupTests.swiftcmuxTests/MarkdownPanelTests.swiftcmuxTests/SettingsSearchIndexTests.swiftcmuxUITests/SettingsAppBehaviorUITests.swiftdocs/cli-contract.mddocs/configuration.mdweb/data/cmux-shortcuts.tsweb/data/cmux.schema.jsonweb/messages/ar.jsonweb/messages/bs.jsonweb/messages/da.jsonweb/messages/de.jsonweb/messages/en.jsonweb/messages/es.jsonweb/messages/fr.jsonweb/messages/it.jsonweb/messages/ja.jsonweb/messages/km.jsonweb/messages/ko.jsonweb/messages/no.jsonweb/messages/pl.jsonweb/messages/pt-BR.jsonweb/messages/ru.jsonweb/messages/th.jsonweb/messages/tr.jsonweb/messages/uk.jsonweb/messages/zh-CN.jsonweb/messages/zh-TW.json
| case .navigation: return "Navigation" | ||
| case .panes: return "Panes" | ||
| case .browser: return "Browser & Find" | ||
| case .markdown: return "Markdown Viewer" |
There was a problem hiding this comment.
Localize the new shortcut metadata.
These new Group.title / displayName values are hard-coded English, and KeyboardShortcutsSection renders action.displayName directly, so the Markdown shortcut rows stay untranslated even when the rest of Settings is localized.
As per coding guidelines, "For production user-facing text, fail partial localization: Swift text must use localized APIs with matching translated string-catalog entries."
Also applies to: 231-233
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Packages/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction.swift` at
line 121, The hard-coded English titles for shortcut metadata (e.g., the
.markdown case returning "Markdown Viewer" in ShortcutAction.swift and the
similar strings at the other noted cases) must be replaced with localized
strings and corresponding catalog entries; update the Group.title / displayName
providers in ShortcutAction (and the other occurrences referenced at 231-233) to
use a localization API (e.g., NSLocalizedString or String(localized:)) with
unique keys like "ShortcutAction.Markdown.title" /
"ShortcutAction.Markdown.displayName", then add matching entries to the app's
Localizable.strings (or the SwiftGen/localizable catalog) so
KeyboardShortcutsSection rendering action.displayName shows translated text.
Ensure keys are descriptive and consistent across all cases.
| .init(section: .app, id: "preferred-editor", title: "Open Files With", synonyms: "app.preferredEditor editor open file code vscode visual studio zed sublime subl cursor"), | ||
| .init(section: .app, id: "supported-file-previews", title: "Open Supported Files in cmux", synonyms: "app.openSupportedFilesInCmux cmd click file preview pdf image video audio quicklook quick look editor external"), | ||
| .init(section: .app, id: "markdown-viewer", title: "Open Markdown in cmux Viewer", synonyms: "app.openMarkdownInCmuxViewer md markdown mdx viewer preview readme"), | ||
| .init(section: .app, id: "markdown-font-size", title: "Markdown Viewer Font Size", synonyms: "app.markdownViewerFontSize markdown viewer font size text scale zoom points preview"), |
There was a problem hiding this comment.
Don't add English-only curated setting entries.
This new curated entry hard-codes both the visible search-result title and its search terms in English, so localized users will see an untranslated result here and won't get locale-appropriate search matches for the new setting.
As per coding guidelines, "For production user-facing text, fail partial localization: Swift text must use localized APIs with matching translated string-catalog entries."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Packages/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/CuratedSettingEntry`+Default.swift
at line 44, The curated entry created via CuratedSettingEntry.init(section:
.app, id: "markdown-font-size", title: "Markdown Viewer Font Size", synonyms:
"app.markdownViewerFontSize markdown viewer font size text scale zoom points
preview") must not hard-code English; replace the literal title and synonyms
with localized lookups (e.g., NSLocalizedString or localized string key API) and
add corresponding keys/values to the app's Localizable.strings catalog for all
supported locales. Update the CuratedSettingEntry creation to reference the
localized title key and a localized synonyms string (or localized array) and
ensure matching entries exist in the strings catalog so both the visible title
and search terms are localized consistently.
| :root { | ||
| color-scheme: light dark; | ||
| --cmux-markdown-font-size: 15px; | ||
| --cmux-markdown-code-font-size: 13.5px; | ||
| --cmux-markdown-small-code-font-size: 12.5px; | ||
| } |
There was a problem hiding this comment.
Frontmatter toggle still ignores the new zoom setting.
The body/error/frontmatter code now scale with --cmux-markdown-font-size, but .cmux-frontmatter summary is still hard-coded at 13px, so enlarged markdown panels keep the only frontmatter affordance tiny and inconsistent.
♻️ Proposed fix
.cmux-frontmatter summary {
cursor: pointer;
display: flex;
align-items: center;
gap: 8px;
padding: 9px 12px;
- font-size: 13px;
+ font-size: calc(var(--cmux-markdown-font-size) * 0.87);
font-weight: 600;
user-select: none;
}Also applies to: 39-40, 210-216
🤖 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 `@Resources/markdown-viewer/shell.html` around lines 16 - 21, The frontmatter
summary is hard-coded to 13px and doesn't respect the zoom variable; update the
CSS rules for .cmux-frontmatter summary (and the other occurrences noted) to
replace the fixed "13px" with the scaling variable, e.g. use
var(--cmux-markdown-font-size) (or a small calc() like
calc(var(--cmux-markdown-font-size) * 0.87) if you want it slightly smaller) so
the frontmatter affordance scales with the panel; apply the same change to the
other referenced selectors at lines ~39-40 and ~210-216.
| func isAvailable(focusedBrowserPanel: Bool, focusedMarkdownPanel: Bool, rightSidebarFocused: Bool) -> Bool { | ||
| switch self { | ||
| case .application: | ||
| return true | ||
| case .nonBrowserPanel: | ||
| return !focusedBrowserPanel && !rightSidebarFocused | ||
| case .browserPanel: | ||
| return focusedBrowserPanel | ||
| case .markdownPanel: | ||
| return focusedMarkdownPanel | ||
| case .rightSidebarFocus: | ||
| return rightSidebarFocused | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and display the relevant Swift file with line numbers.
FILE="Sources/KeyboardShortcutContext.swift"
if [ ! -f "$FILE" ]; then
echo "Missing $FILE"
exit 1
fi
echo "=== File: $FILE (lines 1-120) ==="
sed -n '1,120p' "$FILE" | nl -ba
echo "=== Find definition/usages of rightSidebarFocused ==="
rg -n "rightSidebarFocused" "$FILE"
echo "=== Find isAvailable callers/usage context ==="
rg -n "isAvailable\\(" -S "$FILE"
echo "=== Show any enum/struct around the switch cases ==="
# Show a slightly larger window around the isAvailable function.
START=1
END=120
nl -ba "$FILE" | sed -n "${START},${END}p"Repository: manaflow-ai/cmux
Length of output: 188
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="Sources/KeyboardShortcutContext.swift"
if [ ! -f "$FILE" ]; then
echo "Missing $FILE"
echo "Top-level files:"
ls -la
exit 1
fi
echo "=== File: $FILE (lines 1-160) ==="
sed -n '1,160p' "$FILE" | cat -n
echo "=== Find definition/usages of rightSidebarFocused ==="
rg -n "rightSidebarFocused" "$FILE" || true
echo "=== Find isAvailable( ==="
rg -n "func isAvailable\\(" "$FILE" || true
echo "=== Show window around isAvailable switch ==="
# Print around where isAvailable is defined
LINE="$(rg -n "func isAvailable\\(" "$FILE" | head -n1 | cut -d: -f1)"
if [ -z "${LINE}" ]; then
echo "Could not locate isAvailable"
exit 0
fi
START=$((LINE-30))
END=$((LINE+60))
if [ $START -lt 1 ]; then START=1; fi
sed -n "${START},${END}p" "$FILE" | cat -nRepository: manaflow-ai/cmux
Length of output: 11203
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "shouldRouteRightSidebarModeShortcut" -S Sources || true
rg -n "markdownZoom(In|Out|Reset)" -S Sources || true
rg -n "isAvailable\\(" -S Sources/KeyboardShortcutContext.swift Sources || true
rg -n "shortcutContext" -S Sources/KeyboardShortcutContext.swift Sources || trueRepository: manaflow-ai/cmux
Length of output: 7040
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the key sections around the reported line numbers.
FILE="Sources/AppDelegate.swift"
echo "=== shouldRouteRightSidebarModeShortcut (lines 6320-6405) ==="
sed -n '6320,6405p' "$FILE" | cat -n
echo "=== matchConfiguredShortcut for markdownZoom* (lines 13180-13270) ==="
sed -n '13180,13270p' "$FILE" | cat -n
echo "=== Shortcut availability gate using action.shortcutContext.isAvailable (lines 14370-14430) ==="
sed -n '14370,14430p' "$FILE" | cat -n
echo "=== Another availability check (lines 12610-12650) ==="
sed -n '12610,12660p' "$FILE" | cat -n
echo "=== Any logging conditions for browser.focus.addressBar.shortcutContext (just in case) (lines 13540-13690) ==="
sed -n '13540,13690p' "$FILE" | cat -nRepository: manaflow-ai/cmux
Length of output: 22319
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="Sources/KeyboardShortcutContext.swift"
rg -n "func shortcutEventFocusedMarkdownPanel\\(" "$FILE" || true
LINE="$(rg -n "func shortcutEventFocusedMarkdownPanel\\(" "$FILE" | head -n1 | cut -d: -f1 || true)"
if [ -z "${LINE:-}" ]; then
echo "Could not find shortcutEventFocusedMarkdownPanel"
exit 0
fi
START=$((LINE-40))
END=$((LINE+120))
if [ $START -lt 1 ]; then START=1; fi
sed -n "${START},${END}p" "$FILE" | cat -nRepository: manaflow-ai/cmux
Length of output: 7069
Confirm sidebar-focus behavior for markdown zoom shortcuts
markdownZoomIn/Out/Reset maps to .markdownPanel, and ShortcutContext.isAvailable for .markdownPanel only checks focusedMarkdownPanel—it ignores rightSidebarFocused. Since focusedMarkdownPanel is derived from the selected workspace’s focusedPanelId (not from the current responder), markdown zoom can fire even while the right sidebar has focus. If that suppression is desired, gate it:
Optional gate on right-sidebar focus
case .markdownPanel:
- return focusedMarkdownPanel
+ return focusedMarkdownPanel && !rightSidebarFocused📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func isAvailable(focusedBrowserPanel: Bool, focusedMarkdownPanel: Bool, rightSidebarFocused: Bool) -> Bool { | |
| switch self { | |
| case .application: | |
| return true | |
| case .nonBrowserPanel: | |
| return !focusedBrowserPanel && !rightSidebarFocused | |
| case .browserPanel: | |
| return focusedBrowserPanel | |
| case .markdownPanel: | |
| return focusedMarkdownPanel | |
| case .rightSidebarFocus: | |
| return rightSidebarFocused | |
| } | |
| } | |
| func isAvailable(focusedBrowserPanel: Bool, focusedMarkdownPanel: Bool, rightSidebarFocused: Bool) -> Bool { | |
| switch self { | |
| case .application: | |
| return true | |
| case .nonBrowserPanel: | |
| return !focusedBrowserPanel && !rightSidebarFocused | |
| case .browserPanel: | |
| return focusedBrowserPanel | |
| case .markdownPanel: | |
| return focusedMarkdownPanel && !rightSidebarFocused | |
| case .rightSidebarFocus: | |
| return rightSidebarFocused | |
| } | |
| } |
🤖 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/KeyboardShortcutContext.swift` around lines 27 - 40, The
.markdownPanel availability currently ignores rightSidebarFocused so
markdownZoomIn/Out/Reset (mapped to .markdownPanel) can fire when the right
sidebar is focused; update KeyboardShortcutContext.isAvailable so the
.markdownPanel branch also checks that rightSidebarFocused is false (e.g.,
return focusedMarkdownPanel && !rightSidebarFocused) to suppress markdown zoom
while the right sidebar is focused; ensure you only change the .markdownPanel
case and leave other cases (.application, .nonBrowserPanel, .browserPanel,
.rightSidebarFocus) unchanged.
| "surfacesBlurb": "Поверхности — это вкладки внутри панели.", | ||
| "splitPanes": "Разделённые панели", | ||
| "browser": "Браузер", | ||
| "markdown": "Markdown Viewer", |
There was a problem hiding this comment.
Localize the new markdown category label in Russian.
docs.keyboardShortcuts.cat.markdown is still English in a high-confidence locale file (ru.json), which leaves this category partially untranslated in Russian UI.
Suggested value: "Просмотрщик Markdown" (or your preferred Russian product wording).
Based on learnings: only high-confidence locales (including ru) are expected to have translated copy for changed strings.
🤖 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 `@web/messages/ru.json` at line 635, Replace the English label for
docs.keyboardShortcuts.cat.markdown in the Russian locale with the translated
Russian text (e.g., "Просмотрщик Markdown") so the key
docs.keyboardShortcuts.cat.markdown is fully localized in the ru locale file;
update the value where "markdown": "Markdown Viewer" (or the equivalent
docs.keyboardShortcuts.cat.markdown entry) appears to the Russian string.
| "surfacesBlurb": "Yüzeyler, bir panel içindeki sekmelerdir.", | ||
| "splitPanes": "Bölünmüş Paneller", | ||
| "browser": "Tarayıcı", | ||
| "markdown": "Markdown Viewer", |
There was a problem hiding this comment.
Translate the new markdown category label in Turkish.
The new docs.keyboardShortcuts.cat.markdown entry is English in tr.json; this introduces partial localization for a user-facing category label.
Suggested value: "Markdown Görüntüleyici" (or your preferred Turkish terminology).
Based on learnings: high-confidence locales (including tr) are expected to contain translated copy for newly changed keys.
🤖 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 `@web/messages/tr.json` at line 635, Update the Turkish translation for the
keyboard shortcuts category key docs.keyboardShortcuts.cat.markdown from
"Markdown Viewer" to a Turkish label (suggested "Markdown Görüntüleyici" or
equivalent) so the locale contains a translated user-facing category string;
locate the docs.keyboardShortcuts.cat.markdown entry and replace the English
value with the Turkish translation.
| "surfacesBlurb": "Поверхні — це вкладки всередині панелі.", | ||
| "splitPanes": "Розділені панелі", | ||
| "browser": "Браузер", | ||
| "markdown": "Markdown Viewer", |
There was a problem hiding this comment.
Localize this new shortcut category label for Ukrainian.
Line 636 uses English ("Markdown Viewer") in a high-confidence locale file (uk.json). This should be Ukrainian to satisfy the full i18n requirement for production user-facing copy.
As per coding guidelines: “For production changes, fail … user-facing data changes … update every locale listed in web/i18n/routing.ts with matching web/messages/ entries.”
🤖 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 `@web/messages/uk.json` at line 636, The "markdown" message key in the
Ukrainian locale file (uk.json) is still English ("Markdown Viewer"); replace
its value with the correct Ukrainian translation for the shortcut category label
under the "markdown" key, making sure the string is properly escaped/quoted and
preserves any punctuation; also update the corresponding message entries in
every other locale listed in web/i18n/routing.ts so all locales remain in sync
with this new shortcut category.
| "surfacesBlurb": "Surface 是面板内的标签页。", | ||
| "splitPanes": "分屏面板", | ||
| "browser": "浏览器", | ||
| "markdown": "Markdown Viewer", |
There was a problem hiding this comment.
Translate this category label in zh-CN instead of shipping English fallback.
Line 635 introduces "Markdown Viewer" in English for a high-confidence Chinese locale. Please provide localized Simplified Chinese copy for this key.
Based on learnings: “Only the explicitly high-confidence locales—… zh-Hans, zh-Hant …—are expected to contain fully translated copy for strings changed in a given PR.”
🤖 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 `@web/messages/zh-CN.json` at line 635, The zh-CN locale file currently
contains an English fallback for the "markdown" key ("Markdown Viewer"); replace
that value with a Simplified Chinese translation (e.g., "Markdown 查看器" or
"Markdown 预览器") so the "markdown" key in web/messages/zh-CN.json is localized
for zh-CN; ensure the JSON string value remains properly quoted and escaped.
| "surfacesBlurb": "Surface 是窗格內的分頁。", | ||
| "splitPanes": "分割窗格", | ||
| "browser": "瀏覽器", | ||
| "markdown": "Markdown Viewer", |
There was a problem hiding this comment.
Use Traditional Chinese translation for this new markdown category.
Line 635 currently adds English copy ("Markdown Viewer") in zh-TW. For high-confidence locales, this should be localized Traditional Chinese text.
Based on learnings: “Only the explicitly high-confidence locales—… zh-Hans, zh-Hant …—are expected to contain fully translated copy for strings changed in a given PR.”
🤖 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 `@web/messages/zh-TW.json` at line 635, The zh-TW locale currently has the
English string for the "markdown" key; replace the value of the "markdown" entry
with a Traditional Chinese translation (e.g. "Markdown 檢視器") so the "markdown"
key in web/messages/zh-TW.json contains localized Traditional Chinese text
rather than English.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 56 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="web/messages/bs.json">
<violation number="1" location="web/messages/bs.json:635">
P3: The newly added Bosnian keyboard-shortcut category label is left in English, causing mixed-language UI text.</violation>
</file>
<file name="web/messages/fr.json">
<violation number="1" location="web/messages/fr.json:635">
P3: The new French keyboard-shortcut category label is not localized and appears in English.</violation>
</file>
<file name="web/messages/ar.json">
<violation number="1" location="web/messages/ar.json:635">
P3: The new Arabic translation key uses an English label (`"Markdown Viewer"`), causing a localization regression in the Arabic shortcuts category list.</violation>
</file>
<file name="Sources/ContentView.swift">
<violation number="1" location="Sources/ContentView.swift:6442">
P2: Markdown-scoped shortcut hints are unreachable because markdown focus is hard-coded to false in the availability check.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| guard action.shortcutContext.isAvailable(focusedBrowserPanel: context.bool(CommandPaletteContextKeys.panelIsBrowser), rightSidebarFocused: false) else { | ||
| guard action.shortcutContext.isAvailable( | ||
| focusedBrowserPanel: context.bool(CommandPaletteContextKeys.panelIsBrowser), | ||
| focusedMarkdownPanel: false, |
There was a problem hiding this comment.
P2: Markdown-scoped shortcut hints are unreachable because markdown focus is hard-coded to false in the availability check.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/ContentView.swift, line 6442:
<comment>Markdown-scoped shortcut hints are unreachable because markdown focus is hard-coded to false in the availability check.</comment>
<file context>
@@ -6437,7 +6437,11 @@ struct ContentView: View {
- guard action.shortcutContext.isAvailable(focusedBrowserPanel: context.bool(CommandPaletteContextKeys.panelIsBrowser), rightSidebarFocused: false) else {
+ guard action.shortcutContext.isAvailable(
+ focusedBrowserPanel: context.bool(CommandPaletteContextKeys.panelIsBrowser),
+ focusedMarkdownPanel: false,
+ rightSidebarFocused: false
+ ) else {
</file context>
| "surfacesBlurb": "Površine su tabovi unutar panela.", | ||
| "splitPanes": "Podijeljeni paneli", | ||
| "browser": "Preglednik", | ||
| "markdown": "Markdown Viewer", |
There was a problem hiding this comment.
P3: The newly added Bosnian keyboard-shortcut category label is left in English, causing mixed-language UI text.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/messages/bs.json, line 635:
<comment>The newly added Bosnian keyboard-shortcut category label is left in English, causing mixed-language UI text.</comment>
<file context>
@@ -632,6 +632,7 @@
"surfacesBlurb": "Površine su tabovi unutar panela.",
"splitPanes": "Podijeljeni paneli",
"browser": "Preglednik",
+ "markdown": "Markdown Viewer",
"notifications": "Notifikacije",
"find": "Pretraga",
</file context>
| "markdown": "Markdown Viewer", | |
| "markdown": "Preglednik Markdowna", |
| "surfacesBlurb": "Les surfaces sont des onglets à l'intérieur d'un panneau.", | ||
| "splitPanes": "Panneaux divisés", | ||
| "browser": "Navigateur", | ||
| "markdown": "Markdown Viewer", |
There was a problem hiding this comment.
P3: The new French keyboard-shortcut category label is not localized and appears in English.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/messages/fr.json, line 635:
<comment>The new French keyboard-shortcut category label is not localized and appears in English.</comment>
<file context>
@@ -632,6 +632,7 @@
"surfacesBlurb": "Les surfaces sont des onglets à l'intérieur d'un panneau.",
"splitPanes": "Panneaux divisés",
"browser": "Navigateur",
+ "markdown": "Markdown Viewer",
"notifications": "Notifications",
"find": "Recherche",
</file context>
| "markdown": "Markdown Viewer", | |
| "markdown": "Visionneuse Markdown", |
| "surfacesBlurb": "الأسطح هي علامات تبويب داخل اللوح.", | ||
| "splitPanes": "الألواح المقسمة", | ||
| "browser": "المتصفح", | ||
| "markdown": "Markdown Viewer", |
There was a problem hiding this comment.
P3: The new Arabic translation key uses an English label ("Markdown Viewer"), causing a localization regression in the Arabic shortcuts category list.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/messages/ar.json, line 635:
<comment>The new Arabic translation key uses an English label (`"Markdown Viewer"`), causing a localization regression in the Arabic shortcuts category list.</comment>
<file context>
@@ -632,6 +632,7 @@
"surfacesBlurb": "الأسطح هي علامات تبويب داخل اللوح.",
"splitPanes": "الألواح المقسمة",
"browser": "المتصفح",
+ "markdown": "Markdown Viewer",
"notifications": "الإشعارات",
"find": "البحث",
</file context>
| "markdown": "Markdown Viewer", | |
| "markdown": "عارض Markdown", |
bde31ef to
622ad8b
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmuxUITests/SettingsAppBehaviorUITests.swift (1)
102-108:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
markdownViewerFontSizetotouchedKeysfor proper test isolation.The test resets
markdownViewerFontSizeinline at Line 255, but the key is not in thetouchedKeysarray. This breaks the establishedsetUp/tearDownisolation pattern—tearDownwon't clean up the key after the test, leaving modified state for subsequent tests.🔧 Proposed fix
private static let touchedKeys = [ "workspacePresentationMode", // Minimal Mode (default .standard) "workspaceInheritWorkingDirectory", // Inherit CWD (default true) "menuBarOnly", // Menu Bar Only (default false) "showMenuBarExtra", // Show in Menu Bar (gated row) "commandPalette.switcherSearchAllSurfaces", // Palette all surfaces (default false) + "markdownViewerFontSize", // Markdown viewer font size (default 15) ]Then remove the inline
resetDefaults(["markdownViewerFontSize"])at Line 255, assetUp()will handle it.Also applies to: 255-255
🤖 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 `@cmuxUITests/SettingsAppBehaviorUITests.swift` around lines 102 - 108, The tests modify the "markdownViewerFontSize" preference but it's not listed in the static touchedKeys array, so tearDown won't clean it up; add "markdownViewerFontSize" to the touchedKeys array (the static let touchedKeys in SettingsAppBehaviorUITests) and then remove the inline resetDefaults(["markdownViewerFontSize"]) call at the test's Line 255 so setUp/tearDown isolation handles the reset.cmuxTests/MarkdownPanelTests.swift (1)
230-251:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd the new
fontSizePointsargument to both renderer initializers.Line 236 and Line 248 still call
MarkdownWebRenderer(...)with the old signature, so this test target no longer compiles after the new required parameter was added.🛠 Proposed fix
let firstRenderer = MarkdownWebRenderer( markdown: "# Existing\n", theme: theme, backgroundColor: .windowBackgroundColor, panelId: panelId, workspaceId: workspaceId, filePath: filePath, + fontSizePoints: 15, session: session, onRequestPanelFocus: {} ) @@ let recreatedRenderer = MarkdownWebRenderer( markdown: "# Existing\n", theme: theme, backgroundColor: .windowBackgroundColor, panelId: panelId, workspaceId: workspaceId, filePath: filePath, + fontSizePoints: 15, session: session, onRequestPanelFocus: {} )🤖 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 `@cmuxTests/MarkdownPanelTests.swift` around lines 230 - 251, The test fails because the required fontSizePoints parameter was added to MarkdownWebRenderer; update the two constructor calls for firstRenderer and recreatedRenderer to pass the appropriate fontSizePoints value (e.g., the same font size used elsewhere in tests or a default like theme.baseFontSize) so the initializers match the new MarkdownWebRenderer signature and makeCoordinator remains usable.
♻️ Duplicate comments (6)
web/messages/ru.json (1)
635-635:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLocalize the markdown category label in Russian.
docs.keyboardShortcuts.cat.markdownis still English in a high-confidence locale, so Russian users will see a partially untranslated shortcuts page. Based on learnings: only the explicitly high-confidence locales—includingru—are expected to contain fully translated copy for strings changed in a given PR.🤖 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 `@web/messages/ru.json` at line 635, Replace the English label "Markdown Viewer" in the Russian locale by translating the key docs.keyboardShortcuts.cat.markdown in web/messages/ru.json; update the value for "markdown" to the appropriate Russian string (e.g., "Просмотр Markdown" or your preferred translation) so the shortcuts page is fully localized for the ru locale.web/messages/uk.json (1)
636-636:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLocalize this markdown category label in Ukrainian.
docs.keyboardShortcuts.cat.markdownis still English in a high-confidence locale, so the keyboard-shortcuts page ships partially untranslated for Ukrainian users. Based on learnings: only the explicitly high-confidence locales—includinguk—are expected to contain fully translated copy for strings changed in a given PR.🤖 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 `@web/messages/uk.json` at line 636, The docs.keyboardShortcuts.cat.markdown entry is still in English; update the Ukrainian locale value for that key (docs.keyboardShortcuts.cat.markdown) in uk.json by replacing "Markdown Viewer" with a proper Ukrainian translation (e.g., "Переглядач Markdown") so the keyboard-shortcuts page is fully localized for uk.Packages/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/CuratedSettingEntry+Default.swift (1)
44-44:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't add this curated settings entry as English-only.
The new title and synonyms are user-facing Settings search text, so localized users will see an untranslated result and won't get locale-appropriate search matches for this setting. Route both through localized strings before shipping this entry. As per coding guidelines: “For production user-facing text, fail partial localization: Swift text must use localized APIs with matching translated string-catalog entries.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/CuratedSettingEntry`+Default.swift at line 44, The literal user-facing title and synonyms for the CuratedSettingEntry with id "markdown-font-size" are hard-coded in the .init(...) call; replace them with localized lookups (e.g., NSLocalizedString or Swift String(localized:) keys) instead of English string literals, and add matching entries to the app's Localizable.strings (or StringsDict) catalog for the title and each synonym so localized users see translated search text; update the .init call in CuratedSettingEntry+Default.swift to pass the localized title and localized synonyms values (preserving id "markdown-font-size" and section .app) and ensure you use the same localization keys/comments used by your localization tooling.web/messages/tr.json (1)
635-635:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
keyboardShortcuts.cat.markdownis still untranslated in Turkish.Please replace
"Markdown Viewer"with Turkish copy to keep this high-confidence locale fully localized.Based on learnings: high-confidence locales (including
tr) are expected to contain translated copy for changed keys.🤖 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 `@web/messages/tr.json` at line 635, Replace the untranslated English string for the keyboard shortcut category key keyboardShortcuts.cat.markdown in the tr locale by changing the value "Markdown Viewer" to its Turkish translation (e.g., "Markdown Görüntüleyici"); update the entry for "markdown" inside the messages/tr.json file so the key keyboardShortcuts.cat.markdown contains the Turkish text and save the file to ensure the tr locale is fully localized.web/messages/ko.json (1)
635-635:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
keyboardShortcuts.cat.markdownis still untranslated in Korean.Please replace
"Markdown Viewer"with Korean copy (for example,"마크다운 뷰어") so the shortcuts UI is fully localized forko.Based on learnings: high-confidence locales (including
ko) are expected to contain translated copy for changed keys.🤖 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 `@web/messages/ko.json` at line 635, keyboardShortcuts.cat.markdown entry in web/messages/ko.json is still in English; replace the value "Markdown Viewer" with the Korean translation (e.g., "마크다운 뷰어") for the key keyboardShortcuts.cat.markdown so the shortcuts UI is fully localized for ko, ensuring the JSON string uses proper UTF-8 encoding and matches surrounding punctuation/formatting.Sources/App/WorkspaceRuntimeSettings.swift (1)
103-105:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce the same 1pt minimum in validation and normalization.
minimumPointsis1, butisUsable(_:)still accepts any positive value. That meanscmux.jsoncan import0.5, and the managed default will persist0.5as usable, whileresolved()immediately clamps the effective runtime value back to1. The stored value then disagrees with the actual value in use and falls outside the documented1...96contract.🔧 Proposed fix
static func isUsable(_ points: Double) -> Bool { - points.isFinite && points > 0 && points <= maximumPoints + points.isFinite && points >= minimumPoints && points <= maximumPoints }🤖 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/App/WorkspaceRuntimeSettings.swift` around lines 103 - 105, isUsable(_:) currently allows any positive finite value while minimumPoints is 1, causing stored values (e.g., 0.5) to be considered "usable" even though resolved() clamps them to 1; update isUsable(_:) to enforce the same bounds as normalization by checking points.isFinite && points >= minimumPoints && points <= maximumPoints (use the minimumPoints and maximumPoints constants) so imported/stored values cannot fall outside the documented 1...96 range.
🤖 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 `@cmuxUITests/SettingsAppBehaviorUITests.swift`:
- Around line 254-276: Update the test function
testMarkdownViewerFontSizeStepperRendersDefaultValue to not only assert the
stepper (accessed via window.steppers["SettingsMarkdownViewerFontSizeStepper"])
exists and shows "15 pt", but also tap its increment and decrement controls and
assert the window.staticTexts value updates to "16 pt" and "14 pt" respectively;
after changing the value, optionally relaunch via
makeLaunchedApp()/openAppSection and assert the persisted value remains changed
to verify stored-value propagation; keep using resetDefaults, poll, and
closeSettings utilities to locate elements and wait for UI updates.
In `@Packages/CmuxSettings/Sources/CmuxSettings/Keys/AppCatalogSection.swift`:
- Around line 82-86: Add a Swift-DocC triple-slash comment for the new public
DefaultsKey symbol `markdownViewerFontSize` in `AppCatalogSection.swift`; the
comment should briefly describe the setting's purpose (controls the markdown
viewer font size), note the default value (15) and the userDefaults key
(`markdownViewerFontSize`), and include usage guidance (units/type: Double) so
the public symbol `markdownViewerFontSize: DefaultsKey<Double>` is properly
documented per package guidelines.
In
`@Packages/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/KeyboardShortcutsSection.swift`:
- Around line 293-329: Restoring a shortcut currently bypasses the new
context-aware conflict check and can persist conflicts; update the restore flow
so restoreBinding (and any restore path around lines 346-348) invokes the same
conflict detection used for fresh recordings: call the existing detectConflict
routine (or replicate its logic) using shortcutContext(for:) and
ShortcutConflictContext.overlaps(_:) before applying/persisting the restored
binding, and abort the restore (show error/skip persist) when a conflict is
detected so restored shortcuts pass through the same gate as recorded ones.
In `@README.md`:
- Around line 200-205: Update the "Markdown Viewer" section to clarify these
keyboard shortcuts are scoped to the markdown viewer when it has focus: modify
the heading or add a one-line note above the table (referring to the "Markdown
Viewer" section and the table rows containing the shortcuts "⌘ = / ⌘ -" and "⌘
0") to state that these bindings apply only while the markdown viewer is
focused, so they are not global and won’t conflict with terminal shortcuts.
In `@Sources/AppDelegate.swift`:
- Around line 13233-13243: The current branches call
matchConfiguredShortcut(...) and then immediately return
shortcutEventMarkdownPanel(event)?.zoomIn()/zoomOut()/resetZoom() ?? false,
which consumes the shortcut even when shortcutEventMarkdownPanel(event) is nil;
change each branch to first check that shortcutEventMarkdownPanel(event) returns
a non-nil panel and only then call panel.zoomIn()/zoomOut()/resetZoom() and
return that result; if the panel is nil, do not return/consume — allow the event
to fall through to other handlers. Ensure you reference
matchConfiguredShortcut(event:action:), shortcutEventMarkdownPanel(event), and
the panel methods zoomIn/zoomOut/resetZoom in the fix.
In `@Sources/Panels/MarkdownWebRenderer.swift`:
- Around line 291-309: The applyFontSize(_:) implementation is serializing
computed sizes with "px" but the setting/CLI contract is in points; update
payload values to use "pt" units instead of "px" and keep the same size
calculations (use MarkdownViewerFontSizeSettings.normalized and .rounded for
body, code and small-code sizes), so change the unit suffixes for the three
payload entries created in applyFontSize(_:), and leave the rest (JSON
serialization and webView.evaluateJavaScript) unchanged.
In `@Sources/SessionPersistence.swift`:
- Around line 1554-1557: SessionMarkdownPanelSnapshot currently decodes
fontSizePoints without validation, so implement a custom Decodable initializer
(init(from:)) or failable initializer for SessionMarkdownPanelSnapshot to
normalize fontSizePoints on decode: if the decoded value is within 1...96 keep
it, if outside clamp it to the nearest bound or set to nil (follow project
convention) so restored snapshots enforce the same 1–96 contract as
settings/CLI; specifically update the SessionMarkdownPanelSnapshot decoding path
to perform the clamp/drop logic for the fontSizePoints property.
In `@Sources/TerminalController.swift`:
- Around line 12252-12255: The error message for invalid font size is a raw
English literal; replace it with a localized lookup (e.g. use NSLocalizedString
or your app's localization helper) keyed by
"markdown.open.error.invalidFontSize" in the return from the guard failure in
the v2 font_size handling (inside the v2HasNonNullParam / v2Double /
MarkdownViewerFontSizeSettings.isUsable branch), and add the
"markdown.open.error.invalidFontSize" entry to Resources/Localizable.xcstrings
for every supported locale with the appropriate translations.
In `@web/data/cmux-shortcuts.ts`:
- Around line 201-224: The three new shortcut entries under id "markdown"
(shortcut ids: "markdownZoomIn", "markdownZoomOut", "markdownZoomReset")
currently hardcode description/note objects with only en/ja; replace those
inline strings with locale message keys consumed via next-intl (e.g., use
message ids like "shortcuts.markdown.zoomIn.description" and
"shortcuts.markdown.zoomIn.note") and remove the embedded en/ja objects, then
add the corresponding keys and translations into web/messages/* for every routed
locale (or extend the data model to accept all routed locales), and update
web/i18n/routing.ts to include these message keys so the localized values are
pulled at runtime.
In `@web/data/cmux.schema.json`:
- Around line 322-328: The schema property "markdownViewerFontSize" currently
contains raw English text in its "description"; replace that with a
"descriptionKey" (e.g. "cmux.markdownViewerFontSize.description") and remove the
inline "description" text, then add matching localized entries for that key in
the locale message files for every locale listed in web/i18n/routing.ts (ensure
keys follow existing naming conventions and include the same copy for the
default locale). Update any tests or tooling that validate locale coverage if
present.
In `@web/messages/de.json`:
- Line 635: The German locale key "markdown" in de.json currently contains
English fallback "Markdown Viewer"; update the value to a proper German
translation (e.g., "Markdown-Viewer") so the "markdown" key in de.json provides
a localized string for the German high-confidence locale.
---
Outside diff comments:
In `@cmuxTests/MarkdownPanelTests.swift`:
- Around line 230-251: The test fails because the required fontSizePoints
parameter was added to MarkdownWebRenderer; update the two constructor calls for
firstRenderer and recreatedRenderer to pass the appropriate fontSizePoints value
(e.g., the same font size used elsewhere in tests or a default like
theme.baseFontSize) so the initializers match the new MarkdownWebRenderer
signature and makeCoordinator remains usable.
In `@cmuxUITests/SettingsAppBehaviorUITests.swift`:
- Around line 102-108: The tests modify the "markdownViewerFontSize" preference
but it's not listed in the static touchedKeys array, so tearDown won't clean it
up; add "markdownViewerFontSize" to the touchedKeys array (the static let
touchedKeys in SettingsAppBehaviorUITests) and then remove the inline
resetDefaults(["markdownViewerFontSize"]) call at the test's Line 255 so
setUp/tearDown isolation handles the reset.
---
Duplicate comments:
In
`@Packages/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/CuratedSettingEntry`+Default.swift:
- Line 44: The literal user-facing title and synonyms for the
CuratedSettingEntry with id "markdown-font-size" are hard-coded in the
.init(...) call; replace them with localized lookups (e.g., NSLocalizedString or
Swift String(localized:) keys) instead of English string literals, and add
matching entries to the app's Localizable.strings (or StringsDict) catalog for
the title and each synonym so localized users see translated search text; update
the .init call in CuratedSettingEntry+Default.swift to pass the localized title
and localized synonyms values (preserving id "markdown-font-size" and section
.app) and ensure you use the same localization keys/comments used by your
localization tooling.
In `@Sources/App/WorkspaceRuntimeSettings.swift`:
- Around line 103-105: isUsable(_:) currently allows any positive finite value
while minimumPoints is 1, causing stored values (e.g., 0.5) to be considered
"usable" even though resolved() clamps them to 1; update isUsable(_:) to enforce
the same bounds as normalization by checking points.isFinite && points >=
minimumPoints && points <= maximumPoints (use the minimumPoints and
maximumPoints constants) so imported/stored values cannot fall outside the
documented 1...96 range.
In `@web/messages/ko.json`:
- Line 635: keyboardShortcuts.cat.markdown entry in web/messages/ko.json is
still in English; replace the value "Markdown Viewer" with the Korean
translation (e.g., "마크다운 뷰어") for the key keyboardShortcuts.cat.markdown so the
shortcuts UI is fully localized for ko, ensuring the JSON string uses proper
UTF-8 encoding and matches surrounding punctuation/formatting.
In `@web/messages/ru.json`:
- Line 635: Replace the English label "Markdown Viewer" in the Russian locale by
translating the key docs.keyboardShortcuts.cat.markdown in web/messages/ru.json;
update the value for "markdown" to the appropriate Russian string (e.g.,
"Просмотр Markdown" or your preferred translation) so the shortcuts page is
fully localized for the ru locale.
In `@web/messages/tr.json`:
- Line 635: Replace the untranslated English string for the keyboard shortcut
category key keyboardShortcuts.cat.markdown in the tr locale by changing the
value "Markdown Viewer" to its Turkish translation (e.g., "Markdown
Görüntüleyici"); update the entry for "markdown" inside the messages/tr.json
file so the key keyboardShortcuts.cat.markdown contains the Turkish text and
save the file to ensure the tr locale is fully localized.
In `@web/messages/uk.json`:
- Line 636: The docs.keyboardShortcuts.cat.markdown entry is still in English;
update the Ukrainian locale value for that key
(docs.keyboardShortcuts.cat.markdown) in uk.json by replacing "Markdown Viewer"
with a proper Ukrainian translation (e.g., "Переглядач Markdown") so the
keyboard-shortcuts page is fully localized for uk.
🪄 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: efbd8f57-29e0-44ea-a7ac-9276c71053c1
📒 Files selected for processing (56)
CLI/cmux.swiftPackages/CmuxSettings/Sources/CmuxSettings/Keys/AppCatalogSection.swiftPackages/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction+Defaults.swiftPackages/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction.swiftPackages/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/CuratedSettingEntry+Default.swiftPackages/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/AppSection.swiftPackages/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/KeyboardShortcutsSection.swiftPackages/CmuxSettingsUI/Tests/CmuxSettingsUITests/SettingsRowAnchorResolutionTests.swiftREADME.mdResources/Localizable.xcstringsResources/markdown-viewer/shell.htmlSources/App/WorkspaceRuntimeSettings.swiftSources/AppDelegate.swiftSources/CmuxSettingsJSONPathSupport.swiftSources/ContentView.swiftSources/KeyboardShortcutContext.swiftSources/KeyboardShortcutSettings.swiftSources/KeyboardShortcutSettingsFileStore+Template.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/Panels/MarkdownPanel.swiftSources/Panels/MarkdownPanelView.swiftSources/Panels/MarkdownWebRenderer.swiftSources/SessionPersistence.swiftSources/SettingsNavigation.swiftSources/SettingsSearchAliases.swiftSources/TerminalController.swiftSources/Workspace.swiftcmuxTests/KeyboardShortcutContextTests.swiftcmuxTests/KeyboardShortcutSettingsFileStoreStartupTests.swiftcmuxTests/MarkdownPanelTests.swiftcmuxTests/SettingsSearchIndexTests.swiftcmuxUITests/SettingsAppBehaviorUITests.swiftdocs/cli-contract.mddocs/configuration.mdweb/data/cmux-shortcuts.tsweb/data/cmux.schema.jsonweb/messages/ar.jsonweb/messages/bs.jsonweb/messages/da.jsonweb/messages/de.jsonweb/messages/en.jsonweb/messages/es.jsonweb/messages/fr.jsonweb/messages/it.jsonweb/messages/ja.jsonweb/messages/km.jsonweb/messages/ko.jsonweb/messages/no.jsonweb/messages/pl.jsonweb/messages/pt-BR.jsonweb/messages/ru.jsonweb/messages/th.jsonweb/messages/tr.jsonweb/messages/uk.jsonweb/messages/zh-CN.jsonweb/messages/zh-TW.json
| func testMarkdownViewerFontSizeStepperRendersDefaultValue() { | ||
| resetDefaults(["markdownViewerFontSize"]) | ||
|
|
||
| let app = makeLaunchedApp() | ||
| let window = openAppSection(app) | ||
|
|
||
| let stepper = window.steppers["SettingsMarkdownViewerFontSizeStepper"] | ||
| let scrollView = window.scrollViews.firstMatch | ||
| for _ in 0..<8 where !stepper.exists { | ||
| scrollView.swipeUp() | ||
| } | ||
|
|
||
| XCTAssertTrue( | ||
| poll(timeout: 4.0) { stepper.exists }, | ||
| "Markdown Viewer Font Size stepper should render in App Settings" | ||
| ) | ||
| XCTAssertTrue( | ||
| poll(timeout: 4.0) { window.staticTexts["15 pt"].exists }, | ||
| "Markdown Viewer Font Size should show its default 15 pt value" | ||
| ) | ||
|
|
||
| closeSettings(app, window) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
Consider verifying the effect of changing the stepper, not just its presence.
This test asserts the stepper exists and renders the default "15 pt" value, but it doesn't verify that adjusting the stepper actually updates the persisted setting or has any observable effect. The other TIER 1 tests in this file (e.g., testMinimalModeToggleSwapsSubtitle) toggle the control and assert that the effect propagates—subtitle text changes, enabled state flips, etc.
To match that pattern, consider clicking the stepper increment/decrement buttons and asserting that the displayed value updates (e.g., "16 pt" or "14 pt"), confirming the two-way binding and stored-value propagation.
🤖 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 `@cmuxUITests/SettingsAppBehaviorUITests.swift` around lines 254 - 276, Update
the test function testMarkdownViewerFontSizeStepperRendersDefaultValue to not
only assert the stepper (accessed via
window.steppers["SettingsMarkdownViewerFontSizeStepper"]) exists and shows "15
pt", but also tap its increment and decrement controls and assert the
window.staticTexts value updates to "16 pt" and "14 pt" respectively; after
changing the value, optionally relaunch via makeLaunchedApp()/openAppSection and
assert the persisted value remains changed to verify stored-value propagation;
keep using resetDefaults, poll, and closeSettings utilities to locate elements
and wait for UI updates.
| public let markdownViewerFontSize = DefaultsKey<Double>( | ||
| id: "app.markdownViewerFontSize", | ||
| defaultValue: 15, | ||
| userDefaultsKey: "markdownViewerFontSize" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Document the new public settings key.
markdownViewerFontSize is a new public symbol in a package, but it has no Swift-DocC comment.
📝 Proposed fix
+ /// Default font size, in points, for markdown viewer panels.
public let markdownViewerFontSize = DefaultsKey<Double>(
id: "app.markdownViewerFontSize",
defaultValue: 15,
userDefaultsKey: "markdownViewerFontSize"
)As per coding guidelines, “Every public symbol in new Swift packages under Packages/ must be documented with Swift-DocC triple-slash comment at time of writing.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public let markdownViewerFontSize = DefaultsKey<Double>( | |
| id: "app.markdownViewerFontSize", | |
| defaultValue: 15, | |
| userDefaultsKey: "markdownViewerFontSize" | |
| ) | |
| /// Default font size, in points, for markdown viewer panels. | |
| public let markdownViewerFontSize = DefaultsKey<Double>( | |
| id: "app.markdownViewerFontSize", | |
| defaultValue: 15, | |
| userDefaultsKey: "markdownViewerFontSize" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Packages/CmuxSettings/Sources/CmuxSettings/Keys/AppCatalogSection.swift`
around lines 82 - 86, Add a Swift-DocC triple-slash comment for the new public
DefaultsKey symbol `markdownViewerFontSize` in `AppCatalogSection.swift`; the
comment should briefly describe the setting's purpose (controls the markdown
viewer font size), note the default value (15) and the userDefaults key
(`markdownViewerFontSize`), and include usage guidance (units/type: Double) so
the public symbol `markdownViewerFontSize: DefaultsKey<Double>` is properly
documented per package guidelines.
| private enum ShortcutConflictContext: Equatable { | ||
| case application | ||
| case nonBrowserPanel | ||
| case browserPanel | ||
| case markdownPanel | ||
| case rightSidebarFocus | ||
|
|
||
| func overlaps(_ other: ShortcutConflictContext) -> Bool { | ||
| if self == .application || other == .application { | ||
| return true | ||
| } | ||
| if self == .nonBrowserPanel && other == .markdownPanel { | ||
| return true | ||
| } | ||
| if self == .markdownPanel && other == .nonBrowserPanel { | ||
| return true | ||
| } | ||
| return self == other | ||
| } | ||
| } | ||
|
|
||
| private static func shortcutContext(for action: ShortcutAction) -> ShortcutConflictContext { | ||
| switch action { | ||
| case .switchRightSidebarToFiles, .switchRightSidebarToFind, .switchRightSidebarToSessions, | ||
| .switchRightSidebarToFeed, .switchRightSidebarToDock: | ||
| return .rightSidebarFocus | ||
| case .renameTab, .renameWorkspace: | ||
| return .nonBrowserPanel | ||
| case .browserBack, .browserForward, .browserReload, .toggleBrowserDeveloperTools, | ||
| .showBrowserJavaScriptConsole, .browserZoomIn, .browserZoomOut, .browserZoomReset: | ||
| return .browserPanel | ||
| case .markdownZoomIn, .markdownZoomOut, .markdownZoomReset: | ||
| return .markdownPanel | ||
| default: | ||
| return .application | ||
| } | ||
| } |
There was a problem hiding this comment.
Run restores through the same conflict gate.
The new context-aware detectConflict path only protects fresh recordings. A restored shortcut still goes straight through restoreBinding, so users can clear action A, bind its old chord to overlapping action B, then hit Restore on A and persist a conflict the recorder is supposed to reject.
🔧 Suggested fix
private func restoreBinding(_ shortcut: StoredShortcut, for action: ShortcutAction) async {
+ if let conflict = detectConflict(for: action, stroke: shortcut) {
+ conflictRejections[action.rawValue] = conflict
+ bareKeyRejections.remove(action.rawValue)
+ return
+ }
var updated = bindings
updated[action.rawValue] = shortcut
restoreShortcuts.removeValue(forKey: action.rawValue)
conflictRejections.removeValue(forKey: action.rawValue)
await write(updated)Also applies to: 346-348
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Packages/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/KeyboardShortcutsSection.swift`
around lines 293 - 329, Restoring a shortcut currently bypasses the new
context-aware conflict check and can persist conflicts; update the restore flow
so restoreBinding (and any restore path around lines 346-348) invokes the same
conflict detection used for fresh recordings: call the existing detectConflict
routine (or replicate its logic) using shortcutContext(for:) and
ShortcutConflictContext.overlaps(_:) before applying/persisting the restored
binding, and abort the restore (show error/skip persist) when a conflict is
detected so restored shortcuts pass through the same gate as recorded ones.
| ### Markdown Viewer | ||
|
|
||
| | Shortcut | Action | | ||
| |----------|--------| | ||
| | ⌘ = / ⌘ - | Increase / decrease rendered markdown size | | ||
| | ⌘ 0 | Reset rendered markdown size | |
There was a problem hiding this comment.
Clarify that these shortcuts only apply when a markdown viewer is focused.
Right now this reads as a global binding and conflicts with the terminal ⌘ - / ⌘ 0 shortcuts documented below. Adding the focus scope here would make the README match the routed behavior described in the PR.
Suggested wording
- | ⌘ = / ⌘ - | Increase / decrease rendered markdown size |
- | ⌘ 0 | Reset rendered markdown size |
+ | ⌘ = / ⌘ - | Increase / decrease rendered markdown size in the focused markdown viewer |
+ | ⌘ 0 | Reset rendered markdown size in the focused markdown viewer |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### Markdown Viewer | |
| | Shortcut | Action | | |
| |----------|--------| | |
| | ⌘ = / ⌘ - | Increase / decrease rendered markdown size | | |
| | ⌘ 0 | Reset rendered markdown size | | |
| ### Markdown Viewer | |
| | Shortcut | Action | | |
| |----------|--------| | |
| | ⌘ = / ⌘ - | Increase / decrease rendered markdown size in the focused markdown viewer | | |
| | ⌘ 0 | Reset rendered markdown size in the focused markdown viewer | |
🤖 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 `@README.md` around lines 200 - 205, Update the "Markdown Viewer" section to
clarify these keyboard shortcuts are scoped to the markdown viewer when it has
focus: modify the heading or add a one-line note above the table (referring to
the "Markdown Viewer" section and the table rows containing the shortcuts "⌘ = /
⌘ -" and "⌘ 0") to state that these bindings apply only while the markdown
viewer is focused, so they are not global and won’t conflict with terminal
shortcuts.
| if matchConfiguredShortcut(event: event, action: .markdownZoomIn) { | ||
| return shortcutEventMarkdownPanel(event)?.zoomIn() ?? false | ||
| } | ||
|
|
||
| if matchConfiguredShortcut(event: event, action: .markdownZoomOut) { | ||
| return shortcutEventMarkdownPanel(event)?.zoomOut() ?? false | ||
| } | ||
|
|
||
| if matchConfiguredShortcut(event: event, action: .markdownZoomReset) { | ||
| return shortcutEventMarkdownPanel(event)?.resetZoom() ?? false | ||
| } |
There was a problem hiding this comment.
Let non-Markdown surfaces fall through to later shortcut handlers.
These branches return false as soon as the Markdown binding matches, even when shortcutEventMarkdownPanel(event) is nil. That means an overlapping binding never reaches the browser zoom handlers below, so the shortcut is no longer truly scoped to the focused Markdown panel.
Suggested fix
- if matchConfiguredShortcut(event: event, action: .markdownZoomIn) {
- return shortcutEventMarkdownPanel(event)?.zoomIn() ?? false
+ if matchConfiguredShortcut(event: event, action: .markdownZoomIn),
+ let panel = shortcutEventMarkdownPanel(event) {
+ return panel.zoomIn()
}
- if matchConfiguredShortcut(event: event, action: .markdownZoomOut) {
- return shortcutEventMarkdownPanel(event)?.zoomOut() ?? false
+ if matchConfiguredShortcut(event: event, action: .markdownZoomOut),
+ let panel = shortcutEventMarkdownPanel(event) {
+ return panel.zoomOut()
}
- if matchConfiguredShortcut(event: event, action: .markdownZoomReset) {
- return shortcutEventMarkdownPanel(event)?.resetZoom() ?? false
+ if matchConfiguredShortcut(event: event, action: .markdownZoomReset),
+ let panel = shortcutEventMarkdownPanel(event) {
+ return panel.resetZoom()
}🤖 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 13233 - 13243, The current branches
call matchConfiguredShortcut(...) and then immediately return
shortcutEventMarkdownPanel(event)?.zoomIn()/zoomOut()/resetZoom() ?? false,
which consumes the shortcut even when shortcutEventMarkdownPanel(event) is nil;
change each branch to first check that shortcutEventMarkdownPanel(event) returns
a non-nil panel and only then call panel.zoomIn()/zoomOut()/resetZoom() and
return that result; if the panel is nil, do not return/consume — allow the event
to fall through to other handlers. Ensure you reference
matchConfiguredShortcut(event:action:), shortcutEventMarkdownPanel(event), and
the panel methods zoomIn/zoomOut/resetZoom in the fix.
| struct SessionMarkdownPanelSnapshot: Codable, Sendable { | ||
| var filePath: String | ||
| var fontSizePoints: Double? | ||
| } |
There was a problem hiding this comment.
Enforce the 1–96 font-size contract when restoring snapshots.
fontSizePoints is persisted without bounds checks, so session restore can accept out-of-range values and bypass the same validation enforced in settings/CLI paths. Add normalization in SessionMarkdownPanelSnapshot decode/init (clamp to 1...96, or drop invalid values to nil) to keep restore behavior consistent.
As per coding guidelines: apply cross-file contract checks and fail partial localization/runtime contract breaks where production behavior can diverge.
🤖 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/SessionPersistence.swift` around lines 1554 - 1557,
SessionMarkdownPanelSnapshot currently decodes fontSizePoints without
validation, so implement a custom Decodable initializer (init(from:)) or
failable initializer for SessionMarkdownPanelSnapshot to normalize
fontSizePoints on decode: if the decoded value is within 1...96 keep it, if
outside clamp it to the nearest bound or set to nil (follow project convention)
so restored snapshots enforce the same 1–96 contract as settings/CLI;
specifically update the SessionMarkdownPanelSnapshot decoding path to perform
the clamp/drop logic for the fontSizePoints property.
| if v2HasNonNullParam(params, "font_size") { | ||
| guard let rawFontSize = v2Double(params, "font_size"), | ||
| MarkdownViewerFontSizeSettings.isUsable(rawFontSize) else { | ||
| return .err(code: "invalid_params", message: "font_size must be a positive number no larger than 96", data: nil) |
There was a problem hiding this comment.
Localize the new invalid_params message.
This adds a new user-facing API error body as a raw English literal, so it bypasses the string catalog for every non-English locale.
🌐 Suggested fix
- return .err(code: "invalid_params", message: "font_size must be a positive number no larger than 96", data: nil)
+ return .err(
+ code: "invalid_params",
+ message: String(
+ localized: "markdown.open.error.invalidFontSize",
+ defaultValue: "font_size must be a positive number no larger than 96"
+ ),
+ data: nil
+ )Then add markdown.open.error.invalidFontSize to Resources/Localizable.xcstrings for every supported locale.
As per coding guidelines, "For production user-facing text, fail partial localization: Swift text must use localized APIs with matching translated string-catalog entries."
🤖 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/TerminalController.swift` around lines 12252 - 12255, The error
message for invalid font size is a raw English literal; replace it with a
localized lookup (e.g. use NSLocalizedString or your app's localization helper)
keyed by "markdown.open.error.invalidFontSize" in the return from the guard
failure in the v2 font_size handling (inside the v2HasNonNullParam / v2Double /
MarkdownViewerFontSizeSettings.isUsable branch), and add the
"markdown.open.error.invalidFontSize" entry to Resources/Localizable.xcstrings
for every supported locale with the appropriate translations.
| { | ||
| id: "markdown", | ||
| titleKey: "markdown", | ||
| shortcuts: [ | ||
| { | ||
| id: "markdownZoomIn", | ||
| combos: [["⌘", "="]], | ||
| description: { en: "Zoom markdown in", ja: "Markdown を拡大" }, | ||
| note: { en: "focused markdown viewer", ja: "フォーカス中の Markdown ビューア" }, | ||
| }, | ||
| { | ||
| id: "markdownZoomOut", | ||
| combos: [["⌘", "-"]], | ||
| description: { en: "Zoom markdown out", ja: "Markdown を縮小" }, | ||
| note: { en: "focused markdown viewer", ja: "フォーカス中の Markdown ビューア" }, | ||
| }, | ||
| { | ||
| id: "markdownZoomReset", | ||
| combos: [["⌘", "0"]], | ||
| description: { en: "Reset markdown size", ja: "Markdown サイズをリセット" }, | ||
| note: { en: "focused markdown viewer", ja: "フォーカス中の Markdown ビューア" }, | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
Move these new markdown shortcut strings into the locale system.
This adds three new user-facing shortcut descriptions/notes with only en/ja, so every other supported web locale will still fall back here even though the category label was localized elsewhere. Please source these strings from next-intl/web/messages (or expand the data model to all routed locales) before merge. As per coding guidelines, “web UI text, API response copy, user-facing web data, metadata, route copy, rendered markdown, changelog copy, or message keys must be consumed from next-intl or another locale-specific source and represented across all locales in web/i18n/routing.ts and matching files in web/messages/.”
🤖 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 `@web/data/cmux-shortcuts.ts` around lines 201 - 224, The three new shortcut
entries under id "markdown" (shortcut ids: "markdownZoomIn", "markdownZoomOut",
"markdownZoomReset") currently hardcode description/note objects with only
en/ja; replace those inline strings with locale message keys consumed via
next-intl (e.g., use message ids like "shortcuts.markdown.zoomIn.description"
and "shortcuts.markdown.zoomIn.note") and remove the embedded en/ja objects,
then add the corresponding keys and translations into web/messages/* for every
routed locale (or extend the data model to accept all routed locales), and
update web/i18n/routing.ts to include these message keys so the localized values
are pulled at runtime.
| "markdownViewerFontSize": { | ||
| "type": "number", | ||
| "minimum": 1, | ||
| "maximum": 96, | ||
| "default": 15, | ||
| "description": "Default rendered font size, in points, for new cmux markdown viewer panels." | ||
| }, |
There was a problem hiding this comment.
Localize this new schema description.
This adds new user-facing copy directly in web/data/, so non-English schema consumers will fall back to English. Add a descriptionKey here and the matching locale entries instead of introducing raw text in the schema.
💡 Minimal schema-side change
"markdownViewerFontSize": {
"type": "number",
"minimum": 1,
"maximum": 96,
"default": 15,
+ "descriptionKey": "schemaDescriptions.app.markdownViewerFontSize",
"description": "Default rendered font size, in points, for new cmux markdown viewer panels."
},As per coding guidelines, web/data/**/*.json changes must ensure “user-facing schema titles, descriptions, and data copy have matching localized message coverage for every locale listed in web/i18n/routing.ts.”
🤖 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 `@web/data/cmux.schema.json` around lines 322 - 328, The schema property
"markdownViewerFontSize" currently contains raw English text in its
"description"; replace that with a "descriptionKey" (e.g.
"cmux.markdownViewerFontSize.description") and remove the inline "description"
text, then add matching localized entries for that key in the locale message
files for every locale listed in web/i18n/routing.ts (ensure keys follow
existing naming conventions and include the same copy for the default locale).
Update any tests or tooling that validate locale coverage if present.
| "surfacesBlurb": "Oberflächen sind Tabs innerhalb eines Bereichs.", | ||
| "splitPanes": "Geteilte Bereiche", | ||
| "browser": "Browser", | ||
| "markdown": "Markdown Viewer", |
There was a problem hiding this comment.
Translate this new German locale label instead of leaving English fallback.
"Markdown Viewer" is English in de.json; this key should be localized for the German high-confidence locale (e.g., Markdown-Viewer).
Based on learnings: “Only the explicitly high-confidence locales—en, de, fr, es, ja, ko, zh-Hans, zh-Hant, pt-BR, ru, it, pl, uk, tr, da, no—are expected to contain fully translated copy for strings changed in a given PR.”
🤖 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 `@web/messages/de.json` at line 635, The German locale key "markdown" in
de.json currently contains English fallback "Markdown Viewer"; update the value
to a proper German translation (e.g., "Markdown-Viewer") so the "markdown" key
in de.json provides a localized string for the German high-confidence locale.
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 622ad8b. Configure here.
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Stepper setter bypasses model causing async update lag
Medium Severity
setMarkdownFontSize writes to the store via an async Task instead of calling markdownFontSize.set() directly like every other settings binding in this file. The Stepper's getter reads markdownFontSize.current, which won't reflect the new value until the async store write propagates back through observation. This can cause the Stepper to visually stutter or snap back momentarily on each click, since SwiftUI expects binding updates to be synchronous.
Reviewed by Cursor Bugbot for commit 622ad8b. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (10)
Sources/SessionPersistence.swift (1)
1554-1557:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce the 1–96 font-size contract when decoding snapshots.
fontSizePointsis persisted and decoded without validation, so session restore can accept out-of-range values that would be rejected in settings/CLI paths (which enforce 1–96 points per the PR objectives).Add a custom
init(from:)toSessionMarkdownPanelSnapshotthat clamps decodedfontSizePointsto1...96(or sets tonilif invalid), matching the validation enforced elsewhere.🛡️ Proposed fix to add validation on decode
struct SessionMarkdownPanelSnapshot: Codable, Sendable { var filePath: String var fontSizePoints: Double? + + private enum CodingKeys: String, CodingKey { + case filePath + case fontSizePoints + } + + init(filePath: String, fontSizePoints: Double? = nil) { + self.filePath = filePath + self.fontSizePoints = fontSizePoints + } + + init(from decoder: Decoder) throws { + let container = try decoder.container(keyedBy: CodingKeys.self) + filePath = try container.decode(String.self, forKey: .filePath) + if let raw = try container.decodeIfPresent(Double.self, forKey: .fontSizePoints) { + fontSizePoints = (1...96).contains(raw) ? raw : nil + } else { + fontSizePoints = nil + } + } }As per coding guidelines: apply cross-file contract checks and ensure runtime contracts are consistent across all paths.
🤖 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/SessionPersistence.swift` around lines 1554 - 1557, SessionMarkdownPanelSnapshot currently decodes fontSizePoints without validation; add a custom init(from decoder: Decoder) in the SessionMarkdownPanelSnapshot type that decodes the optional Double for fontSizePoints and enforces the 1...96 contract by clamping values inside that range or setting fontSizePoints to nil when the decoded value is outside the valid range or not finite; ensure you still synthesize Codable for other properties by implementing only init(from:) and using a keyed container with the same key name ("fontSizePoints") so restore behavior matches other paths.web/messages/es.json (1)
635-635:⚠️ Potential issue | 🟠 MajorTranslate this new shortcut category in Spanish.
es.jsonis a high-confidence locale, so leaving"Markdown Viewer"in English breaks the full-localization contract for this new user-facing label. Use Spanish copy here instead (for example,Visor de Markdown). As per coding guidelines, "For production user-facing text, fail partial localization."🤖 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 `@web/messages/es.json` at line 635, The JSON entry with key "markdown" currently uses English text "Markdown Viewer"; update the Spanish locale string in web/messages/es.json by replacing the value for the "markdown" key with an appropriate Spanish translation (e.g., "Visor de Markdown") so the label is fully localized and adheres to the "fail partial localization" guideline.Sources/Panels/MarkdownWebRenderer.swift (1)
295-297:⚠️ Potential issue | 🟠 MajorKeep the injected font sizes in points.
The new setting/CLI contract is explicitly in points, but these CSS variables are serialized as
px. That makes the markdown renderer apply a different size than the one the user configured. Useptfor all three injected values instead.💡 Suggested fix
- "--cmux-markdown-font-size": "\(bodySize)px", - "--cmux-markdown-code-font-size": "\(MarkdownViewerFontSizeSettings.rounded(bodySize * 0.9))px", - "--cmux-markdown-small-code-font-size": "\(MarkdownViewerFontSizeSettings.rounded(bodySize * (12.5 / 15.0)))px" + "--cmux-markdown-font-size": "\(bodySize)pt", + "--cmux-markdown-code-font-size": "\(MarkdownViewerFontSizeSettings.rounded(bodySize * 0.9))pt", + "--cmux-markdown-small-code-font-size": "\(MarkdownViewerFontSizeSettings.rounded(bodySize * (12.5 / 15.0)))pt"In CSS/WebKit, how do `pt` and `px` differ for `font-size`, and would `font-size: 15pt` render differently from `15px`?🤖 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/MarkdownWebRenderer.swift` around lines 295 - 297, The injected CSS variables in MarkdownWebRenderer.swift are serialized with "px" but the setting/CLI uses points; update the three variables "--cmux-markdown-font-size", "--cmux-markdown-code-font-size", and "--cmux-markdown-small-code-font-size" to use "pt" instead of "px" when formatting their values so the renderer receives sizes in points consistent with the contract.Packages/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction.swift (1)
121-121:⚠️ Potential issue | 🟠 MajorLocalize the new shortcut metadata.
These new group/display strings are hard-coded English, so the Markdown shortcut section and rows will stay untranslated in the app even when the rest of Settings is localized. Route them through
String(localized:defaultValue:)(or the existing equivalent) and add matching catalog entries. As per coding guidelines, "Swift text must use localized APIs with matching translated string-catalog entries."Also applies to: 231-233
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction.swift` at line 121, The new hard-coded English display/group strings for the Markdown shortcut (case .markdown in ShortcutAction.swift) and the other related strings at the later block (lines ~231-233) must be routed through the localization API; replace the raw literals with String(localized:defaultValue:) (or the project’s existing localization helper) for the enum's display/group names (e.g., the .markdown display string and the corresponding group title/rows) and add matching entries to the app’s strings catalog so translators can provide localized values.web/messages/ko.json (1)
635-635:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLocalize the new markdown category label for Korean.
Line 635 keeps
keyboardShortcuts.cat.markdownin English ("Markdown Viewer"), which leaves mixed-language UI in a high-confidence locale. Please translate it (e.g.,마크다운 뷰어) to keep locale coverage consistent.As per coding guidelines, production web message changes must maintain full localization coverage across locales in
web/messages/.🤖 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 `@web/messages/ko.json` at line 635, The "keyboardShortcuts.cat.markdown" message value is left in English; update the Korean locale entry by translating the value (the "markdown" key) to Korean (for example "마크다운 뷰어") so the entry in web/messages/ko.json is fully localized; locate the "markdown" key in that file and replace the English string with the appropriate Korean translation preserving JSON formatting and quotation marks.web/messages/fr.json (1)
635-635:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTranslate the new markdown category label in French.
Line 635 sets
keyboardShortcuts.cat.markdownto English ("Markdown Viewer"). For a high-confidence locale, this should be localized (for example,Visionneuse Markdown).Based on learnings, high-confidence locales (including
fr) are expected to carry translated copy for changed keys.🤖 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 `@web/messages/fr.json` at line 635, The label for the markdown shortcuts category is still in English; update the JSON key "keyboardShortcuts.cat.markdown" (currently "Markdown Viewer") to a French translation such as "Visionneuse Markdown" or another appropriate French string so the fr locale carries translated copy for this changed key.web/messages/uk.json (1)
636-636:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTranslate the new markdown shortcut category in
uk.json.This high-confidence locale still ships the English label
"Markdown Viewer", so the new user-facing key is only partially localized.🌐 Suggested fix
- "markdown": "Markdown Viewer", + "markdown": "Переглядач Markdown",As per coding guidelines, “For production changes, fail … update every locale listed in
web/i18n/routing.tswith matchingweb/messages/entries.”🤖 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 `@web/messages/uk.json` at line 636, The Ukrainian locale still uses the English label for the new shortcut category; update the "markdown" key in web/messages/uk.json (the entry currently "markdown": "Markdown Viewer") with the correct Ukrainian translation, and ensure the same key is added/updated in every locale referenced by web/i18n/routing.ts so all shipping locales include the matching web/messages/ entry for "markdown".Sources/KeyboardShortcutContext.swift (1)
35-36:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSuppress markdown zoom shortcuts while the right sidebar owns focus.
context.markdownPanelis derived from the selected workspace’sfocusedPanelId, not the current responder, so it stays non-nilafter focus moves into the right sidebar. With Line 36 returning onlyfocusedMarkdownPanel,markdownZoomIn/Out/Resetcan still fire against the background markdown panel while sidebar controls are focused.🔧 Minimal fix
case .markdownPanel: - return focusedMarkdownPanel + return focusedMarkdownPanel && !rightSidebarFocusedAlso applies to: 43-46, 177-186
🤖 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/KeyboardShortcutContext.swift` around lines 35 - 36, The markdownPanel getter returns focusedMarkdownPanel based only on the workspace's focusedPanelId, causing markdownZoomIn/Out/Reset to remain active when the right sidebar actually owns focus; update the logic in the markdownPanel property (and the similar blocks at the other occurrences) to return focusedMarkdownPanel only if the markdown panel is the current responder/owns focus (e.g., check the app's current firstResponder or a "focusedResponderId"/isFirstResponder flag tied to the panel) and otherwise return nil so sidebar focus suppresses the markdown zoom shortcuts.Sources/App/WorkspaceRuntimeSettings.swift (1)
104-106:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce the same 1pt minimum in validation and normalization.
isUsable(_:)still accepts any positive value (points > 0) even thoughminimumPointsis1. This allows out-of-range values like0.5to pass validation and be stored, whilenormalized(_:)(line 114) clamps them to1. The stored value then disagrees with the effective runtime value.🛡️ Proposed fix
static func isUsable(_ points: Double) -> Bool { - points.isFinite && points > 0 && points <= maximumPoints + points.isFinite && points >= minimumPoints && points <= maximumPoints }🤖 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/App/WorkspaceRuntimeSettings.swift` around lines 104 - 106, Update isUsable(_:) to enforce the same minimum as normalization by checking points.isFinite && points >= minimumPoints && points <= maximumPoints so values like 0.5 are rejected; reference the existing minimumPoints and maximumPoints constants and ensure behavior matches normalized(_:) to avoid stored vs runtime discrepancies.Sources/AppDelegate.swift (1)
13233-13243:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLet non-Markdown surfaces fall through to later shortcut handlers.
These branches return
falsewhenshortcutEventMarkdownPanel(event)isnil, which consumes the event and prevents it from reaching the browser zoom handlers below. Guard on a non-nil panel so overlapping shortcuts can fall through when no Markdown panel is focused.Suggested fix
- if matchConfiguredShortcut(event: event, action: .markdownZoomIn) { - return shortcutEventMarkdownPanel(event)?.zoomIn() ?? false + if matchConfiguredShortcut(event: event, action: .markdownZoomIn), + let panel = shortcutEventMarkdownPanel(event) { + return panel.zoomIn() } - if matchConfiguredShortcut(event: event, action: .markdownZoomOut) { - return shortcutEventMarkdownPanel(event)?.zoomOut() ?? false + if matchConfiguredShortcut(event: event, action: .markdownZoomOut), + let panel = shortcutEventMarkdownPanel(event) { + return panel.zoomOut() } - if matchConfiguredShortcut(event: event, action: .markdownZoomReset) { - return shortcutEventMarkdownPanel(event)?.resetZoom() ?? false + if matchConfiguredShortcut(event: event, action: .markdownZoomReset), + let panel = shortcutEventMarkdownPanel(event) { + return panel.resetZoom() }Based on learnings, Markdown/browser shortcut routing in
Sources/AppDelegate.swiftmust stay scoped to the focused surface and avoid bypassing other handlers when the target surface is not active.🤖 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 13233 - 13243, The current branches use "return shortcutEventMarkdownPanel(event)?.zoomIn() ?? false" (and similar for zoomOut/resetZoom) which consumes the event when no Markdown panel is focused; instead, change each branch so you only return when a non-nil panel exists. Concretely, inside the matchConfiguredShortcut checks for .markdownZoomIn, .markdownZoomOut, and .markdownZoomReset, call shortcutEventMarkdownPanel(event) into a local (e.g., guard/if let panel = shortcutEventMarkdownPanel(event)) and only call and return panel.zoomIn()/zoomOut()/resetZoom() when panel is non-nil; if nil, do not return and allow the event to fall through to later handlers.
🤖 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 `@CLI/cmux.swift`:
- Around line 14519-14529: The CLI currently allows font sizes >0 (so 0.5
passes) but the documented/required contract is 1–96 pt; update the validation
in isUsableMarkdownViewerFontSize to require size.isFinite && size >= 1 && size
<= 96 (instead of size > 0) and adjust the parseMarkdownViewerFontSize error
message to reflect "must be between 1 and 96" (or "1–96") so the CLI message
matches the settings/schema; keep using roundedMarkdownViewerMetric(size) on
success.
In `@cmuxTests/MarkdownPanelTests.swift`:
- Line 300: Replace the hardcoded font size 15 used in
coordinator.loadShell(theme:theme, initialMarkdown:..., fontSizePoints: 15)
calls with the constant MarkdownViewerFontSizeSettings.defaultPoints for
consistency; search for occurrences in MarkdownPanelTests.swift (specifically
the coordinator.loadShell calls at the highlighted lines) and update each call
(including the instances at lines referenced in the review) to pass
MarkdownViewerFontSizeSettings.defaultPoints instead of 15.
In `@docs/configuration.md`:
- Around line 25-33: The docs currently say "for new cmux markdown viewer
panels" which implies a one-time default; update the wording for
app.markdownViewerFontSize to state that any markdown viewer panel that does not
have a per-panel override will continue to follow the global
app.markdownViewerFontSize setting as it changes (live-follow behavior), and
clarify that only panels with a per-panel override via the markdown viewer
shortcuts will stop following the app setting until that panel's override is
reset; reference the exact symbol app.markdownViewerFontSize, mention "per-panel
override" and "markdown viewer shortcuts" and add a short sentence that
resetting a panel's override restores live-following of
app.markdownViewerFontSize.
In `@Packages/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/AppSection.swift`:
- Around line 766-777: The setter setMarkdownFontSize(_:) currently only writes
to defaultsStore inside a Task and never updates the in-memory bound model,
causing UI lag; change it to synchronously assign the clamped value to the bound
DefaultsValueModel (markdownFontSize) first, then persist that same clamped
value to defaultsStore (using the existing key and Task/await pattern), and
finally post CmuxSettingsRuntimeNotifications.markdownViewerFontSizeDidChange
from the persisted path (after await) on MainActor so observers see the
persisted change; update references to setMarkdownFontSize, markdownFontSize,
defaultsStore, key, and the NotificationCenter.post call accordingly.
- Around line 758-764: The function formatMarkdownFontSize currently uses
String(format:) which is not locale-aware; replace both the integer and
fractional return paths in formatMarkdownFontSize(_:) to use Swift's
locale-aware FormatStyle (e.g., use
clamped.formatted(.number.precision(.fractionLength(0))) for integer-equivalent
values and clamped.formatted(.number.precision(.fractionLength(2))) for the
fractional case), keeping clamped computed via clampedMarkdownFontSize(_:) and
returning the formatted string.
In `@Sources/ContentView.swift`:
- Around line 6440-6444: The guard call to action.shortcutContext.isAvailable
incorrectly hardcodes focusedMarkdownPanel: false; change it to pass the actual
command-palette markdown focus value (similar to how focusedBrowserPanel is
derived) so markdown-scoped actions show correctly — e.g. replace the hardcoded
false with the palette context accessor
(context.bool(CommandPaletteContextKeys.panelIsMarkdown) or the actual key used
for markdown focus) used alongside CommandPaletteContextKeys.panelIsBrowser.
In `@Sources/Panels/MarkdownPanel.swift`:
- Around line 142-150: zoomIn() and zoomOut() always call setFontSize(...) even
when the computed size is clamped back to the current value, which writes
fontSizeOverridePoints and pins the panel; fix by computing the intended new
size using fontSizePoints +/- MarkdownViewerFontSizeSettings.zoomStepPoints,
compare it to the current fontSizePoints (after applying any clamping logic you
use), and only call setFontSize(...) (and return its result) when the new size
differs from the current size; otherwise return false without touching
fontSizeOverridePoints. Use the existing symbols zoomIn, zoomOut,
fontSizePoints, setFontSize(...), fontSizeOverridePoints, and
MarkdownViewerFontSizeSettings.zoomStepPoints to locate and implement the
change.
In `@Sources/TerminalController.swift`:
- Around line 12322-12323: The code currently falls back to
MarkdownViewerFontSizeSettings.defaultPoints when createdPanel?.fontSizePoints
is nil; instead, return the effective resolved markdown font size by using the
panel's effective value or the app setting. Replace the fallback to
MarkdownViewerFontSizeSettings.defaultPoints with the resolved size from the
created panel (e.g., createdPanel?.effectiveFontSize or
createdPanel?.resolvedFontSize if that accessor exists) or, if no resolved
accessor is available, use the app's configured app.markdownViewerFontSize (not
the static default) so the emitted "font_size" reflects the actual size the
panel is using.
In `@Sources/Workspace.swift`:
- Around line 14724-14733: The current canonical-path scan in panels (used by
openOrFocusMarkdownSplit and openOrFocusMarkdownSurface) applies setFontSize to
the first MarkdownPanel whose resolved filePath matches, which can target the
wrong panel when duplicates exist; update the logic in those functions to prefer
the explicitly requested panel/split target (e.g., a provided paneId or
targetId) by first locating that panel in panels and applying
md.setFontSize(fontSizePoints) only if that panel's resolved path matches, and
only fall back to canonical-path matching if no explicit target was provided;
alternatively, when a specific pane/split target is supplied but no matching
panel exists, create a new panel via newMarkdownSurface / splitPaneWithMarkdown
instead of retargeting an unrelated MarkdownPanel. Ensure references to panels,
MarkdownPanel, setFontSize, focusPanel, newMarkdownSurface,
splitPaneWithMarkdown, openOrFocusMarkdownSplit, and openOrFocusMarkdownSurface
are used to find and update the correct panel.
In `@web/messages/pl.json`:
- Line 635: The Polish locale file contains an untranslated entry: the key
"markdown" currently has the English value "Markdown Viewer"; update the pl.json
entry for "markdown" to a proper Polish translation (e.g., "Podgląd Markdown" or
your approved localization) so the user-facing label is fully localized; modify
the value for the "markdown" key in pl.json and ensure it follows existing
locale formatting (quotes/commas) and passes i18n/CI checks.
---
Duplicate comments:
In `@Packages/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction.swift`:
- Line 121: The new hard-coded English display/group strings for the Markdown
shortcut (case .markdown in ShortcutAction.swift) and the other related strings
at the later block (lines ~231-233) must be routed through the localization API;
replace the raw literals with String(localized:defaultValue:) (or the project’s
existing localization helper) for the enum's display/group names (e.g., the
.markdown display string and the corresponding group title/rows) and add
matching entries to the app’s strings catalog so translators can provide
localized values.
In `@Sources/App/WorkspaceRuntimeSettings.swift`:
- Around line 104-106: Update isUsable(_:) to enforce the same minimum as
normalization by checking points.isFinite && points >= minimumPoints && points
<= maximumPoints so values like 0.5 are rejected; reference the existing
minimumPoints and maximumPoints constants and ensure behavior matches
normalized(_:) to avoid stored vs runtime discrepancies.
In `@Sources/AppDelegate.swift`:
- Around line 13233-13243: The current branches use "return
shortcutEventMarkdownPanel(event)?.zoomIn() ?? false" (and similar for
zoomOut/resetZoom) which consumes the event when no Markdown panel is focused;
instead, change each branch so you only return when a non-nil panel exists.
Concretely, inside the matchConfiguredShortcut checks for .markdownZoomIn,
.markdownZoomOut, and .markdownZoomReset, call shortcutEventMarkdownPanel(event)
into a local (e.g., guard/if let panel = shortcutEventMarkdownPanel(event)) and
only call and return panel.zoomIn()/zoomOut()/resetZoom() when panel is non-nil;
if nil, do not return and allow the event to fall through to later handlers.
In `@Sources/KeyboardShortcutContext.swift`:
- Around line 35-36: The markdownPanel getter returns focusedMarkdownPanel based
only on the workspace's focusedPanelId, causing markdownZoomIn/Out/Reset to
remain active when the right sidebar actually owns focus; update the logic in
the markdownPanel property (and the similar blocks at the other occurrences) to
return focusedMarkdownPanel only if the markdown panel is the current
responder/owns focus (e.g., check the app's current firstResponder or a
"focusedResponderId"/isFirstResponder flag tied to the panel) and otherwise
return nil so sidebar focus suppresses the markdown zoom shortcuts.
In `@Sources/Panels/MarkdownWebRenderer.swift`:
- Around line 295-297: The injected CSS variables in MarkdownWebRenderer.swift
are serialized with "px" but the setting/CLI uses points; update the three
variables "--cmux-markdown-font-size", "--cmux-markdown-code-font-size", and
"--cmux-markdown-small-code-font-size" to use "pt" instead of "px" when
formatting their values so the renderer receives sizes in points consistent with
the contract.
In `@Sources/SessionPersistence.swift`:
- Around line 1554-1557: SessionMarkdownPanelSnapshot currently decodes
fontSizePoints without validation; add a custom init(from decoder: Decoder) in
the SessionMarkdownPanelSnapshot type that decodes the optional Double for
fontSizePoints and enforces the 1...96 contract by clamping values inside that
range or setting fontSizePoints to nil when the decoded value is outside the
valid range or not finite; ensure you still synthesize Codable for other
properties by implementing only init(from:) and using a keyed container with the
same key name ("fontSizePoints") so restore behavior matches other paths.
In `@web/messages/es.json`:
- Line 635: The JSON entry with key "markdown" currently uses English text
"Markdown Viewer"; update the Spanish locale string in web/messages/es.json by
replacing the value for the "markdown" key with an appropriate Spanish
translation (e.g., "Visor de Markdown") so the label is fully localized and
adheres to the "fail partial localization" guideline.
In `@web/messages/fr.json`:
- Line 635: The label for the markdown shortcuts category is still in English;
update the JSON key "keyboardShortcuts.cat.markdown" (currently "Markdown
Viewer") to a French translation such as "Visionneuse Markdown" or another
appropriate French string so the fr locale carries translated copy for this
changed key.
In `@web/messages/ko.json`:
- Line 635: The "keyboardShortcuts.cat.markdown" message value is left in
English; update the Korean locale entry by translating the value (the "markdown"
key) to Korean (for example "마크다운 뷰어") so the entry in web/messages/ko.json is
fully localized; locate the "markdown" key in that file and replace the English
string with the appropriate Korean translation preserving JSON formatting and
quotation marks.
In `@web/messages/uk.json`:
- Line 636: The Ukrainian locale still uses the English label for the new
shortcut category; update the "markdown" key in web/messages/uk.json (the entry
currently "markdown": "Markdown Viewer") with the correct Ukrainian translation,
and ensure the same key is added/updated in every locale referenced by
web/i18n/routing.ts so all shipping locales include the matching web/messages/
entry for "markdown".
🪄 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: 5404234f-d799-4f1d-8bcb-92c94706bc23
📒 Files selected for processing (58)
CLI/cmux.swiftPackages/CmuxSettings/Sources/CmuxSettings/Keys/AppCatalogSection.swiftPackages/CmuxSettings/Sources/CmuxSettings/RuntimeNotifications.swiftPackages/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction+Defaults.swiftPackages/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction.swiftPackages/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/CuratedSettingEntry+Default.swiftPackages/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/AppSection.swiftPackages/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/KeyboardShortcutsSection.swiftPackages/CmuxSettingsUI/Tests/CmuxSettingsUITests/SettingsRowAnchorResolutionTests.swiftREADME.mdResources/Localizable.xcstringsResources/markdown-viewer/shell.htmlSources/App/WorkspaceRuntimeSettings.swiftSources/AppDelegate.swiftSources/CmuxSettingsJSONPathSupport.swiftSources/ContentView.swiftSources/KeyboardShortcutContext.swiftSources/KeyboardShortcutSettings.swiftSources/KeyboardShortcutSettingsFileStore+Template.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/Panels/MarkdownPanel.swiftSources/Panels/MarkdownPanelView.swiftSources/Panels/MarkdownWebRenderer.swiftSources/SessionPersistence.swiftSources/SettingsNavigation.swiftSources/SettingsSearchAliases.swiftSources/TerminalController.swiftSources/Workspace.swiftcmuxTests/KeyboardShortcutContextTests.swiftcmuxTests/KeyboardShortcutSettingsFileStoreStartupTests.swiftcmuxTests/MarkdownPanelTests.swiftcmuxTests/SettingsSearchIndexTests.swiftcmuxUITests/SettingsAppBehaviorUITests.swiftcmuxUITests/SettingsUITestSupport.swiftdocs/cli-contract.mddocs/configuration.mdweb/data/cmux-shortcuts.tsweb/data/cmux.schema.jsonweb/messages/ar.jsonweb/messages/bs.jsonweb/messages/da.jsonweb/messages/de.jsonweb/messages/en.jsonweb/messages/es.jsonweb/messages/fr.jsonweb/messages/it.jsonweb/messages/ja.jsonweb/messages/km.jsonweb/messages/ko.jsonweb/messages/no.jsonweb/messages/pl.jsonweb/messages/pt-BR.jsonweb/messages/ru.jsonweb/messages/th.jsonweb/messages/tr.jsonweb/messages/uk.jsonweb/messages/zh-CN.jsonweb/messages/zh-TW.json
| private func parseMarkdownViewerFontSize(_ rawValue: String) throws -> Double { | ||
| let trimmed = rawValue.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| guard let size = Double(trimmed), | ||
| isUsableMarkdownViewerFontSize(size) else { | ||
| throw CLIError(message: "--font-size must be a positive number no larger than 96") | ||
| } | ||
| return roundedMarkdownViewerMetric(size) | ||
| } | ||
|
|
||
| private func isUsableMarkdownViewerFontSize(_ size: Double) -> Bool { | ||
| size.isFinite && size > 0 && size <= 96 |
There was a problem hiding this comment.
Align --font-size validation with the new 1–96pt contract.
This helper currently accepts any finite value greater than 0, so cmux markdown open --font-size 0.5 passes here even though the new markdown viewer font-size setting is documented as 1–96 pt. That makes the CLI surface inconsistent with the settings/schema layer for the same feature.
Suggested fix
private func parseMarkdownViewerFontSize(_ rawValue: String) throws -> Double {
let trimmed = rawValue.trimmingCharacters(in: .whitespacesAndNewlines)
guard let size = Double(trimmed),
isUsableMarkdownViewerFontSize(size) else {
- throw CLIError(message: "--font-size must be a positive number no larger than 96")
+ throw CLIError(message: "--font-size must be between 1 and 96")
}
return roundedMarkdownViewerMetric(size)
}
private func isUsableMarkdownViewerFontSize(_ size: Double) -> Bool {
- size.isFinite && size > 0 && size <= 96
+ size.isFinite && size >= 1 && size <= 96
}🤖 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 `@CLI/cmux.swift` around lines 14519 - 14529, The CLI currently allows font
sizes >0 (so 0.5 passes) but the documented/required contract is 1–96 pt; update
the validation in isUsableMarkdownViewerFontSize to require size.isFinite &&
size >= 1 && size <= 96 (instead of size > 0) and adjust the
parseMarkdownViewerFontSize error message to reflect "must be between 1 and 96"
(or "1–96") so the CLI message matches the settings/schema; keep using
roundedMarkdownViewerMetric(size) on success.
| defer { coordinator.close() } | ||
|
|
||
| coordinator.loadShell(theme: theme, initialMarkdown: "# Existing\n") | ||
| coordinator.loadShell(theme: theme, initialMarkdown: "# Existing\n", fontSizePoints: 15) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider using MarkdownViewerFontSizeSettings.defaultPoints instead of hardcoded 15 for consistency.
Lines 237 and 250 already use MarkdownViewerFontSizeSettings.defaultPoints. For consistency and to avoid magic numbers, consider replacing hardcoded 15 with the constant in these test coordinator calls.
♻️ Proposed refactor for consistency
- coordinator.loadShell(theme: theme, initialMarkdown: "# Existing\n", fontSizePoints: 15)
+ coordinator.loadShell(theme: theme, initialMarkdown: "# Existing\n", fontSizePoints: MarkdownViewerFontSizeSettings.defaultPoints)Apply the same pattern to lines 319, 327, 340, 356, 369, 377, 389, 394, and 399.
Also applies to: 319-319, 327-327, 340-340, 356-356, 369-369, 377-377, 389-389, 394-394, 399-399
🤖 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 `@cmuxTests/MarkdownPanelTests.swift` at line 300, Replace the hardcoded font
size 15 used in coordinator.loadShell(theme:theme, initialMarkdown:...,
fontSizePoints: 15) calls with the constant
MarkdownViewerFontSizeSettings.defaultPoints for consistency; search for
occurrences in MarkdownPanelTests.swift (specifically the coordinator.loadShell
calls at the highlighted lines) and update each call (including the instances at
lines referenced in the review) to pass
MarkdownViewerFontSizeSettings.defaultPoints instead of 15.
| ## `app.markdownViewerFontSize` | ||
|
|
||
| Controls the default rendered font size for new cmux markdown viewer panels. | ||
|
|
||
| Value: a number of points from `1` through `96`. | ||
|
|
||
| Default: `15`. | ||
|
|
||
| Panel-specific zoom from the markdown viewer shortcuts overrides this value for that panel until reset. |
There was a problem hiding this comment.
Document that non-overridden panels keep following the app default.
“for new cmux markdown viewer panels” makes this sound one-shot, but the feature contract here is that any markdown panel without a per-panel override should continue tracking app.markdownViewerFontSize as it changes. Please capture that live-follow behavior in the docs.
📝 Suggested wording
-Controls the default rendered font size for new cmux markdown viewer panels.
+Controls the default rendered font size for cmux markdown viewer panels that do not have a per-panel zoom override.
@@
-Panel-specific zoom from the markdown viewer shortcuts overrides this value for that panel until reset.
+Panel-specific zoom from the markdown viewer shortcuts overrides this value for that panel until reset. Panels without an override continue following the app default.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## `app.markdownViewerFontSize` | |
| Controls the default rendered font size for new cmux markdown viewer panels. | |
| Value: a number of points from `1` through `96`. | |
| Default: `15`. | |
| Panel-specific zoom from the markdown viewer shortcuts overrides this value for that panel until reset. | |
| ## `app.markdownViewerFontSize` | |
| Controls the default rendered font size for cmux markdown viewer panels that do not have a per-panel zoom override. | |
| Value: a number of points from `1` through `96`. | |
| Default: `15`. | |
| Panel-specific zoom from the markdown viewer shortcuts overrides this value for that panel until reset. Panels without an override continue following the app default. |
🤖 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 `@docs/configuration.md` around lines 25 - 33, The docs currently say "for new
cmux markdown viewer panels" which implies a one-time default; update the
wording for app.markdownViewerFontSize to state that any markdown viewer panel
that does not have a per-panel override will continue to follow the global
app.markdownViewerFontSize setting as it changes (live-follow behavior), and
clarify that only panels with a per-panel override via the markdown viewer
shortcuts will stop following the app setting until that panel's override is
reset; reference the exact symbol app.markdownViewerFontSize, mention "per-panel
override" and "markdown viewer shortcuts" and add a short sentence that
resetting a panel's override restores live-following of
app.markdownViewerFontSize.
| private static func formatMarkdownFontSize(_ value: Double) -> String { | ||
| let clamped = clampedMarkdownFontSize(value) | ||
| if clamped.rounded() == clamped { | ||
| return "\(Int(clamped))" | ||
| } | ||
| return String(format: "%.2f", clamped) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Swift/Foundation, what locale behavior does String(format: "%.2f", value) use for user-facing decimal output, and what locale-aware API is recommended for formatting floating-point values in SwiftUI settings labels?
💡 Result:
The Swift initializer String(format: "%.2f", value) does not automatically respect the user's current locale settings [1]. It uses a C-style printf-based formatting engine that defaults to the "C" locale, meaning it will consistently use a period (.) as the decimal separator regardless of the user's regional preferences, which may expect a comma (,) or other locale-specific conventions [1]. To make String(format:) locale-aware, you must explicitly provide a locale, such as by using the initializer String(format:locale:_:arguments:) [1][2]. For SwiftUI settings labels and general UI output, the recommended approach is to use the modern Swift FormatStyle APIs [3][4]. These APIs are natively locale-aware and handle formatting details automatically [5][6]. Recommended API: The.formatted method with a specific format style is the standard for SwiftUI [5][3]. For example, to format a number to two decimal places while respecting the current user's locale, you can use: Text(value, format:.number.precision(.fractionLength(2))) This approach is preferred because it is declarative, type-safe, and automatically adapts to the user's environment (e.g., decimal separators, grouping separators) without requiring manual locale management [5][6][4].
Citations:
- 1: https://sarunw.com/posts/how-to-specify-fractional-digits-for-formatted-number-string-in-swift/
- 2: https://stackoverflow.com/questions/24051314/precision-string-format-specifier-in-swift
- 3: https://stackoverflow.com/questions/56828331/display-a-float-with-specified-decimal-places-in-swiftui
- 4: https://medium.com/@jpmtech/formatting-numbers-in-swiftui-fc5ee2920a59
- 5: https://sosumi.ai/documentation/foundation/floatingpointformatstyle
- 6: https://developer.apple.com/documentation/foundation/floatingpointformatstyle
Use locale-aware number formatting for displayed font size
String(format: "%.2f", clamped) uses the C/printf-style formatting behavior (not the user’s current locale), so the decimal separator can be wrong for some locales. Format the Double with Swift’s locale-aware FormatStyle (e.g., clamped.formatted(.number.precision(.fractionLength(2)))) and apply the same locale-aware formatting to the integer path too in Packages/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/AppSection.swift (lines 758-764).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Packages/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/AppSection.swift`
around lines 758 - 764, The function formatMarkdownFontSize currently uses
String(format:) which is not locale-aware; replace both the integer and
fractional return paths in formatMarkdownFontSize(_:) to use Swift's
locale-aware FormatStyle (e.g., use
clamped.formatted(.number.precision(.fractionLength(0))) for integer-equivalent
values and clamped.formatted(.number.precision(.fractionLength(2))) for the
fractional case), keeping clamped computed via clampedMarkdownFontSize(_:) and
returning the formatted string.
| private func setMarkdownFontSize(_ value: Double) { | ||
| let clamped = Self.clampedMarkdownFontSize(value) | ||
| let key = catalog.app.markdownViewerFontSize | ||
| Task { [defaultsStore] in | ||
| await defaultsStore.set(clamped, for: key) | ||
| await MainActor.run { | ||
| NotificationCenter.default.post( | ||
| name: CmuxSettingsRuntimeNotifications.markdownViewerFontSizeDidChange, | ||
| object: nil | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Update the bound value model synchronously.
This setter bypasses markdownFontSize and only writes through defaultsStore in a fire-and-forget task. That breaks the file’s own “value-model drives invalidation” contract, so the stepper label/value can lag or snap back until store observation catches up. Route the change through the bound DefaultsValueModel as part of the setter, then post the runtime notification from the persisted path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Packages/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/AppSection.swift`
around lines 766 - 777, The setter setMarkdownFontSize(_:) currently only writes
to defaultsStore inside a Task and never updates the in-memory bound model,
causing UI lag; change it to synchronously assign the clamped value to the bound
DefaultsValueModel (markdownFontSize) first, then persist that same clamped
value to defaultsStore (using the existing key and Task/await pattern), and
finally post CmuxSettingsRuntimeNotifications.markdownViewerFontSizeDidChange
from the persisted path (after await) on MainActor so observers see the
persisted change; update references to setMarkdownFontSize, markdownFontSize,
defaultsStore, key, and the NotificationCenter.post call accordingly.
| guard action.shortcutContext.isAvailable( | ||
| focusedBrowserPanel: context.bool(CommandPaletteContextKeys.panelIsBrowser), | ||
| focusedMarkdownPanel: false, | ||
| rightSidebarFocused: false | ||
| ) else { |
There was a problem hiding this comment.
Thread the real markdown focus state into this availability check.
Hardcoding focusedMarkdownPanel: false makes markdown-scoped actions look unavailable here even when the command palette was opened from a markdown panel, so their shortcut hints disappear. Please pass through the palette’s actual markdown-panel context the same way focusedBrowserPanel is derived.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/ContentView.swift` around lines 6440 - 6444, The guard call to
action.shortcutContext.isAvailable incorrectly hardcodes focusedMarkdownPanel:
false; change it to pass the actual command-palette markdown focus value
(similar to how focusedBrowserPanel is derived) so markdown-scoped actions show
correctly — e.g. replace the hardcoded false with the palette context accessor
(context.bool(CommandPaletteContextKeys.panelIsMarkdown) or the actual key used
for markdown focus) used alongside CommandPaletteContextKeys.panelIsBrowser.
| @discardableResult | ||
| func zoomIn() -> Bool { | ||
| setFontSize(fontSizePoints + MarkdownViewerFontSizeSettings.zoomStepPoints) | ||
| } | ||
|
|
||
| @discardableResult | ||
| func zoomOut() -> Bool { | ||
| setFontSize(fontSizePoints - MarkdownViewerFontSizeSettings.zoomStepPoints) | ||
| } |
There was a problem hiding this comment.
Avoid pinning a panel override on clamped zoom no-ops.
At Line 144 and Line 149, zoomIn() / zoomOut() always call setFontSize(...). When the panel is already at the min/max bound, normalization clamps back to the current size, but setFontSize still writes fontSizeOverridePoints. A panel that was following the app default then stops tracking later default changes even though the zoom keypress did nothing visible.
♻️ Proposed fix
`@discardableResult`
func zoomIn() -> Bool {
- setFontSize(fontSizePoints + MarkdownViewerFontSizeSettings.zoomStepPoints)
+ let next = MarkdownViewerFontSizeSettings.normalized(
+ fontSizePoints + MarkdownViewerFontSizeSettings.zoomStepPoints
+ )
+ guard next != fontSizePoints else { return false }
+ return setFontSize(next)
}
`@discardableResult`
func zoomOut() -> Bool {
- setFontSize(fontSizePoints - MarkdownViewerFontSizeSettings.zoomStepPoints)
+ let next = MarkdownViewerFontSizeSettings.normalized(
+ fontSizePoints - MarkdownViewerFontSizeSettings.zoomStepPoints
+ )
+ guard next != fontSizePoints else { return false }
+ return setFontSize(next)
}🤖 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/MarkdownPanel.swift` around lines 142 - 150, zoomIn() and
zoomOut() always call setFontSize(...) even when the computed size is clamped
back to the current value, which writes fontSizeOverridePoints and pins the
panel; fix by computing the intended new size using fontSizePoints +/-
MarkdownViewerFontSizeSettings.zoomStepPoints, compare it to the current
fontSizePoints (after applying any clamping logic you use), and only call
setFontSize(...) (and return its result) when the new size differs from the
current size; otherwise return false without touching fontSizeOverridePoints.
Use the existing symbols zoomIn, zoomOut, fontSizePoints, setFontSize(...),
fontSizeOverridePoints, and MarkdownViewerFontSizeSettings.zoomStepPoints to
locate and implement the change.
| "path": filePath, | ||
| "font_size": createdPanel?.fontSizePoints ?? MarkdownViewerFontSizeSettings.defaultPoints |
There was a problem hiding this comment.
Return the effective markdown font size, not the built-in default.
If the panel was opened without an explicit override, createdPanel?.fontSizePoints can stay unset while the panel follows the user's configured app.markdownViewerFontSize. Falling back to MarkdownViewerFontSizeSettings.defaultPoints reports 15 even when the active app default is different, which breaks the new CLI/output contract for customized setups. Please source this field from the resolved effective size after panel creation instead of the static default.
🤖 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/TerminalController.swift` around lines 12322 - 12323, The code
currently falls back to MarkdownViewerFontSizeSettings.defaultPoints when
createdPanel?.fontSizePoints is nil; instead, return the effective resolved
markdown font size by using the panel's effective value or the app setting.
Replace the fallback to MarkdownViewerFontSizeSettings.defaultPoints with the
resolved size from the created panel (e.g., createdPanel?.effectiveFontSize or
createdPanel?.resolvedFontSize if that accessor exists) or, if no resolved
accessor is available, use the app's configured app.markdownViewerFontSize (not
the static default) so the emitted "font_size" reflects the actual size the
panel is using.
| let canonical = (filePath as NSString).resolvingSymlinksInPath | ||
| for (existingId, panel) in panels { | ||
| guard let md = panel as? MarkdownPanel else { continue } | ||
| if (md.filePath as NSString).resolvingSymlinksInPath == canonical { | ||
| if let fontSizePoints { | ||
| md.setFontSize(fontSizePoints) | ||
| } | ||
| focusPanel(existingId) | ||
| return md | ||
| } |
There was a problem hiding this comment.
Target the font-size override to the intended panel, not the first path match.
openOrFocusMarkdownSplit and openOrFocusMarkdownSurface now call setFontSize(...) on the first canonical filePath match they find, but this file still allows multiple MarkdownPanels for the same path to be created via newMarkdownSurface(...) / splitPaneWithMarkdown(...). Once duplicates exist, the scan at Line 14725 / Line 14925 can mutate and persist the wrong panel's per-panel override, even when the caller passed a specific pane/split target. Please resolve the match against the requested pane before applying the override, or create a new panel instead of retargeting an unrelated one.
Also applies to: 14924-14934, 14942-14950
🤖 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/Workspace.swift` around lines 14724 - 14733, The current
canonical-path scan in panels (used by openOrFocusMarkdownSplit and
openOrFocusMarkdownSurface) applies setFontSize to the first MarkdownPanel whose
resolved filePath matches, which can target the wrong panel when duplicates
exist; update the logic in those functions to prefer the explicitly requested
panel/split target (e.g., a provided paneId or targetId) by first locating that
panel in panels and applying md.setFontSize(fontSizePoints) only if that panel's
resolved path matches, and only fall back to canonical-path matching if no
explicit target was provided; alternatively, when a specific pane/split target
is supplied but no matching panel exists, create a new panel via
newMarkdownSurface / splitPaneWithMarkdown instead of retargeting an unrelated
MarkdownPanel. Ensure references to panels, MarkdownPanel, setFontSize,
focusPanel, newMarkdownSurface, splitPaneWithMarkdown, openOrFocusMarkdownSplit,
and openOrFocusMarkdownSurface are used to find and update the correct panel.
| "surfacesBlurb": "Surface'y to karty wewnątrz panelu.", | ||
| "splitPanes": "Dzielone panele", | ||
| "browser": "Przeglądarka", | ||
| "markdown": "Markdown Viewer", |
There was a problem hiding this comment.
Translate the new Polish shortcut category label.
Line 635 adds an English value ("Markdown Viewer") in pl.json; this introduces partial localization for a user-facing label in the Polish catalog.
As per coding guidelines, production user-facing text in locale catalogs must be fully localized across supported locales, and placeholder/copied English is disallowed for touched entries.
🤖 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 `@web/messages/pl.json` at line 635, The Polish locale file contains an
untranslated entry: the key "markdown" currently has the English value "Markdown
Viewer"; update the pl.json entry for "markdown" to a proper Polish translation
(e.g., "Podgląd Markdown" or your approved localization) so the user-facing
label is fully localized; modify the value for the "markdown" key in pl.json and
ensure it follows existing locale formatting (quotes/commas) and passes i18n/CI
checks.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary:
Verification:
Note:
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Low Risk
UI/settings and markdown viewer rendering only; zoom shortcuts are context-scoped to avoid browser conflicts, with unit/UI tests covering font-size behavior.
Overview
Adds configurable font sizing for the cmux markdown viewer: a global default (
app.markdownViewerFontSize, 1–96 pt), per-panel zoom/reset, and optional size when opening via CLI.Settings & config: New App setting with stepper, search/docs/schema/README updates,
cmux.jsonimport/export, and a runtime notification when the default changes so panels without a per-panel override pick up the new size.Runtime:
MarkdownPaneltracksfontSizePointsand optional overrides; session snapshots persist overrides. The WKWebView shell uses CSS variables and live JS updates for body/code sizes.markdown.openacceptsfont_size/ CLI--font-sizewith validation.Shortcuts: New markdown-only zoom actions (same chords as browser zoom) routed when a markdown panel is focused; shortcut conflict detection treats markdown vs browser contexts separately so shared defaults do not block binding both.
Reviewed by Cursor Bugbot for commit 622ad8b. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds font-size controls to the markdown viewer with a new app default, per-panel zoom, and a CLI option. Improves readability, scopes zoom to the focused markdown panel, and keeps browser zoom unaffected.
app.markdownViewerFontSize(1–96 pt, default 15) with a Settings control,cmux.jsonsupport, search alias, and docs/schema updates.cmux markdown open --font-size <points>validates input, applies on open, and returnsfont_sizein the response/printout.Written for commit 622ad8b. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Chores
Tests