Skip to content

fix(macos): move Pin off Cmd-P and test menu chords - #174

Merged
SarthakWade merged 11 commits into
mainfrom
feat/macos-shortcuts
Sep 12, 2026
Merged

SarthakWade merged 11 commits into
mainfrom
feat/macos-shortcuts

Conversation

@yashranaway

Copy link
Copy Markdown
Collaborator

Summary

Implements the unblocked part of #160.

  • Menu chords are a catalog in `MenuShortcuts.swift`. The Cocoa host applies that catalog instead of hard-coding modifiers in `buildMenu()`.
  • Pin on Top is Cmd-Option-P. Cmd-P is no longer taken from Print.
  • Snapshot stays Cmd-Shift-S on purpose.
  • Cmd-, is not wired. Settings (macOS Settings window: shared preferences UI and Cmd+, shortcut #158) is parked.
  • Protocol suite rejects duplicate chords and checks the Pin/snapshot/Settings reservations. P0 documents the chords.

Full GUI E2E of every menu item still wants a macOS session. The collision contract is tested without one.

Closes #160

@yashranaway yashranaway self-assigned this Sep 11, 2026
Pin on Top is now Cmd-Option-P. Shortcuts live in a portable catalog so
the protocol suite can reject collisions, including a reserved Cmd-,.

@SarthakWade SarthakWade left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The shortcut collision itself is fixed, but this PR does not yet satisfy the menu behavior contract and has two user-visible correctness bugs.

The catalog is only used to apply key/modifier values by title. Its menu and selector fields do not drive the actual NSMenuItem action or target, and the protocol test only checks static strings, so wrong selectors, targets, responder-chain behavior, or renamed titles still pass. Issue #160 specifically requires those paths and AppKit behavior to be exercised. The start page also still advertises Cmd-P for Pin even though the menu now uses Cmd-Option-P.

Please wire the catalog to the real menu construction or add a testable representation of the actual built menu, update the start-page shortcut, fix the Help chord, and run the macOS menu E2E. This PR touches main.swift but the WKWebView E2E job was skipped because the macos-e2e label was not present.

GitHub currently reports the PR as mergeable with no conflicts, and all executed required checks passed. I found no security vulnerability in the changed code.

Comment thread apps/headless/main.swift Outdated
Comment thread apps/headless/Sources/HeadlessProtocol/MenuShortcuts.swift Outdated
NSMenuItems now take selector, target, and chord from MenuShortcutSpec.
Help is Cmd-Shift-/, and the start page advertises Option-Command-P.
@yashranaway yashranaway added the macos-e2e Run the macOS WKWebView E2E suite label Sep 12, 2026
@yashranaway

Copy link
Copy Markdown
Collaborator Author

Addressed the review.

Menu items now take selector, target, and modifiers from MenuShortcutSpec via NSSelectorFromString. Help is key / with shift (Cmd-Shift-/). The start page advertises Option-Command-P, not Command-P.

Protocol tests cover unique titles, Pin/Help/New Window/Quit selectors and targets, and the start-page copy. macOS E2E greps the host source and probes the live Pin/Help menu chords through Accessibility when the process is reachable. Added the macos-e2e label.

@SarthakWade SarthakWade left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-reviewed the final implementation after integration with current main. The shortcut catalog now constructs the real menu items and covers exact chords, targets, responder-chain editing, window actions, navigation, reload, snapshot isolation, zoom, full screen, history, pin state, and focus restoration. The complete rerun passes protocol/security, runtime, Linux E2E, macOS build, and macOS WKWebView E2E.

@SarthakWade
SarthakWade merged commit 7a2efb5 into main Sep 12, 2026
15 of 16 checks passed
@SarthakWade
SarthakWade deleted the feat/macos-shortcuts branch September 12, 2026 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

macos-e2e Run the macOS WKWebView E2E suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

macOS shortcuts: audit collisions and test menu behavior

2 participants