Skip to content

fix(keybinding): allow remapping Cmd+R captured by View menu - #1734

Closed
BillionClaw wants to merge 2 commits into
manaflow-ai:mainfrom
BillionClaw:clawoss/fix/cmd-r-viewmenu-1773827744
Closed

BillionClaw wants to merge 2 commits into
manaflow-ai:mainfrom
BillionClaw:clawoss/fix/cmd-r-viewmenu-1773827744

Conversation

@BillionClaw

@BillionClaw BillionClaw commented Mar 18, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #1708

Cmd+R was captured by the View menu's 'Reload Page' action and could not be remapped or passed to terminal applications like Neovim.

This change makes the 'Reload Page' shortcut configurable via KeyboardShortcutSettings:

  • Users can now remap the shortcut to a different key combination
  • Users can clear the shortcut entirely to pass Cmd+R to terminal apps
  • The default behavior (Cmd+R reloads browser when focused) is preserved

Changes:

  • Added reloadPage action to KeyboardShortcutSettings with default Cmd+R
  • Changed menu item from hardcoded shortcut to configurable one
  • Added handleCustomShortcut handling that only reloads when browser is focused, passing through to terminal apps otherwise

Summary by cubic

Make the “Reload Page” shortcut configurable and only trigger it when the in-app browser is focused. Users can remap or clear Cmd+R so terminal apps (e.g., Neovim) can receive it.

  • New Features

    • Added reloadPage to KeyboardShortcutSettings (default Cmd+R); users can remap or clear it.
    • View menu now uses the configurable shortcut instead of a hardcoded one.
    • Shortcut only reloads when the browser is focused; otherwise it passes through to terminal apps.
  • Bug Fixes

    • Added missing localizations for the “Reload Page” shortcut label.

Written for commit 6184465. Summary will update on new commits.

Summary by CodeRabbit

  • New Features
    • Added a customizable "Reload Page" keyboard shortcut for the browser panel (default: Cmd+R). Shortcut can be remapped or cleared in settings and is respected when the browser panel is focused.
  • UI / Menu
    • "Reload Page" is exposed in the main menu with its configured shortcut.
  • Localization
    • Added localized label for the "Reload Page" shortcut.

Make the 'Reload Page' shortcut configurable so users can remap or unbind it.
Previously, Cmd+R was hardcoded on the View menu, preventing it from
reaching terminal applications like Neovim.

Changes:
- Add reloadPage action to KeyboardShortcutSettings with default Cmd+R
- Change menu item to use configurable shortcut instead of hardcoded one
- Add handleCustomShortcut handling to only reload when browser is focused,
  passing the key through to terminal apps otherwise

Fixes manaflow-ai#1708

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

@vercel

vercel Bot commented Mar 18, 2026

Copy link
Copy Markdown

@BillionClaw is attempting to deploy a commit to the Manaflow Team on Vercel.

A member of the Team first needs to authorize it.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 3 files

@coderabbitai

coderabbitai Bot commented Mar 18, 2026 •

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 480e2a03-b477-41a0-b87c-e793b14d3ca3

📥 Commits

Reviewing files that changed from the base of the PR and between f64b300 and 6184465.

📒 Files selected for processing (1)
  • Resources/Localizable.xcstrings

📝 Walkthrough

Walkthrough

Adds a configurable "Reload Page" keyboard shortcut (default Cmd+R) and routes it conditionally: if a browser panel is focused the app consumes the shortcut and reloads the page; otherwise the shortcut is not consumed so terminal apps can receive it.

Changes

Cohort / File(s) Summary
Shortcut Settings
Sources/KeyboardShortcutSettings.swift
Adds Action.reloadPage, reloadPageShortcut() helper, localization key mapping, and default Cmd+R stored shortcut.
Shortcut Routing / Event Handling
Sources/AppDelegate.swift
Handles the reloadPage shortcut: checks focused browser panel, calls browserPanel.reload() and consumes the event; returns false to allow passthrough when not a browser.
Menu & Shortcut Storage
Sources/cmuxApp.swift
Replaces hardcoded Reload Page menu item with splitCommandButton wired to a stored reloadPageMenuShortcut and persisted reloadPageShortcutData.
Localization
Resources/Localizable.xcstrings
Adds shortcut.reloadPage.label translations for the new shortcut label across supported locales.

Sequence Diagram

sequenceDiagram
    participant User
    participant AppDelegate
    participant KeyboardShortcutSettings
    participant BrowserPanel
    participant TerminalApp

    User->>AppDelegate: Press Reload shortcut (e.g., Cmd+R)
    AppDelegate->>KeyboardShortcutSettings: Does event match reloadPage?
    alt matches reloadPage
        AppDelegate->>BrowserPanel: Is browser panel focused?
        alt browser focused
            AppDelegate->>BrowserPanel: browserPanel.reload()
            BrowserPanel-->>AppDelegate: reload started
            AppDelegate-->>User: Event consumed
        else not browser
            AppDelegate-->>TerminalApp: Let event pass through
            TerminalApp-->>User: Terminal receives keypress
        end
    else not a reload shortcut
        AppDelegate-->>User: Handle other shortcuts
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

🐰 I hopped through keys to set them free,
Cmd+R now knows where it must be.
If web view sings, I'll press reload,
Else terminals get their secret code.
Hooray — no trapped keystroke, jump and glee! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title accurately describes the main change: making the Cmd+R keybinding configurable to allow remapping in keybinding settings.
Description check ✅ Passed The description covers what changed, why, and references the fixed issue, but lacks testing details, demo video, and checklist completion as specified in the template.
Linked Issues check ✅ Passed All requirements from issue #1708 are met: the shortcut is now configurable via KeyboardShortcutSettings, remappable, clearable, and only reloads when browser is focused, passing through to terminal apps otherwise.
Out of Scope Changes check ✅ Passed All changes are directly related to making the Reload Page shortcut configurable and focused-browser aware, with no unrelated modifications to the codebase.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@Sources/KeyboardShortcutSettings.swift`:
- Line 78: KeyboardShortcutSettings.swift references the new enum case
.reloadPage which uses the localization key "shortcut.reloadPage.label"; add
that key to Resources/Localizable.xcstrings with both English and Japanese
entries following the same pattern as the other shortcut keys (e.g., provide
"Reload Page" for English and the appropriate Japanese translation) so the
lookup in KeyboardShortcutSettings (case .reloadPage) resolves correctly at
runtime.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5f1f97b6-8e5b-495e-8aad-888d95450b23

📥 Commits

Reviewing files that changed from the base of the PR and between 0a99bb5 and f64b300.

📒 Files selected for processing (3)
  • Sources/AppDelegate.swift
  • Sources/KeyboardShortcutSettings.swift
  • Sources/cmuxApp.swift

Comment thread Sources/KeyboardShortcutSettings.swift
@BillionClaw

Copy link
Copy Markdown
Contributor Author

This is BillionClaw. Happy to discuss the approach or make adjustments to the fix.

@BillionClaw

Copy link
Copy Markdown
Contributor Author

Closing per repository blocklist: maintainer threatened to ban. All submissions to this repo have been suspended.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cmd+R captured by View menu "Reload Page" cannot be remapped or passed to terminal apps (e.g. Neovim)

1 participant