ReviewOS

also looking at this

craft-native/craft

fix(shortcuts): make global hotkeys actually fire

#54
Merged glennmichael123 wants to merge fix/global-shortcuts-never-fire into main
11 files +2435 -417
11 days ago

Closes #47.

window.craft.shortcuts.register() reported success and then nothing ever happened. The issue names one cause; there turned out to be four, stacked in the same namespace, each individually fatal:

  1. No monitor was ever installed. setupGlobalMonitor computed an NSEventMask, discarded it, logged "Global monitor setup (polling mode)" — there was no polling either — and returned. The matcher it was meant to feed was reachable only from handleGlobalKeyEvent, which had no callers anywhere in the tree.
  2. The payload shapes disagreed. The JS bridge has always posted {id, accelerator}; the native side read key and modifiers. Every registration failed its own validation long before it reached the monitor that wasn't there.
  3. isRegistered and list answered into the void. They called __craftShortcutList / __craftShortcutRegistered, which nothing defines, while the JS facade waited on __craftBridgeResult. Both promises hung for the full 30-second request timeout and then rejected.
  4. Triggering called an undefined global too. __craftShortcutCallback, while the JS side listened for a craft:shortcut event nothing dispatched.

Breaks 2–4 are the same shape as #27/#29: two halves each correct on their own terms, disagreeing with each other, with tests on both sides that could not see it.

The mechanism

Carbon's RegisterEventHotKey, as the issue suggests. It takes a plain C callback — no Objective-C block, which is where the previous attempt stopped — and unlike a CGEventTap it needs no Accessibility or Input Monitoring permission and only ever calls back for the exact combinations the app asked for. craft never sees any other keystroke.

Registration is by physical key, so a binding survives the user switching to Dvorak.

Two behaviour decisions worth flagging

A bare key is now refused (ModifierRequired), function keys excepted. A global hotkey on H means no application on the system — craft's own windows included — ever sees the user type an h again. A failed registration is recoverable; that is not. Shift alone doesn't count, since Shift+H is how you type a capital H.

disable() releases the key rather than holding the reservation and dropping the event. The alternative would leave that combination dead in every other app for as long as craft ran.

Verification

Beyond unit tests, I checked the Carbon binding against the live API, which turned out to matter:

handler installed: true
register -> anyopaque@100d1b430
duplicate register -> null           # a combination the process already holds is refused
after release, register -> non-null

That third line is why re-registering an id must release first — otherwise register('toggle', 'Cmd+Shift+H') twice (a page reload) silently fails the second time. The tests now pin it.

Delivery is covered by putting a real EventRef through Carbon's own dispatcher, which is the only way to find out whether the handler signature and the EventHotKeyID layout are what Carbon actually expects, short of pressing the key on a Mac:

fired = 42 (expect 42)
after foreign signature, fired = 0 (expect 0)

Control-checked: flipping the parameter type to a bogus FourCC fails that test rather than silently skipping. The one link I could not automate here is the OS's own key→hotkey matching — synthesizing the keystroke needs Accessibility permission this terminal doesn't have.

The JS↔native contract is now carried across the boundary in both directions in injected_js_test.zig, the way it already was for menus. Control check: reverting the JS half fails 5 of them.

While adding those I found _req throws a ReferenceError inside its Promise executor without a setTimeout — so the entire request-shaped half of the bridge had never been exercised headlessly. Stubbed and commented.

Shape

Split so the parts are testable without an app, a window or a key press:

filewhat
key_codes.zigname ↔ virtual keycode, one table read both ways
accelerator.zig"Cmd+Shift+H"
shortcut_registry.zigthe lifecycle, against a fake platform
macos_hotkey.zigCarbon
bridge_shortcuts.zignow a thin adapter (556 lines → ~200)

Linux and Windows refuse registration outright instead of accepting one that can never fire, and craft.shortcuts.onError carries a refusal back to the page. register cannot be a request — its action name collides with serviceMenu's in the pending queue, and enable/disable with autoLaunch's — so an event is the honest surface.

bun test 468 pass, zig build test clean, tsc --noEmit clean, pickier clean.

11 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.