Enable fn key for transcription on macos and Escape key for cancelling non-PTT transcriptions #2
Conversation
WalkthroughAdds macOS Fn-key monitoring, new macOS Cargo deps, routing in the shortcut layer to use an Fn monitor, dynamic runtime registration for a Cancel binding, UI support to set the fn binding, and adjusted cancel flow to discard recordings without triggering transcription. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend as Settings UI
participant Shortcut as Shortcut System
participant FnMonitor as Fn Monitor (macOS)
participant EventStream as macOS Event Stream
User->>Frontend: Click "Use fn" button
Frontend->>Shortcut: register_binding("fn")
Shortcut->>Shortcut: is_fn_binding() -> true
Shortcut->>FnMonitor: register_fn_binding(binding_id)
FnMonitor->>EventStream: Install NSEvent FlagsChanged listener (main thread)
EventStream-->>FnMonitor: FlagsChanged events (Fn press/release)
FnMonitor->>Shortcut: dispatch_binding_event(binding_id, "fn", Pressed/Released)
Shortcut->>Frontend: notify registration success
sequenceDiagram
participant User
participant EventStream as macOS Event Stream
participant FnMonitor as Fn Monitor
participant Shortcut as Shortcut System
participant Cancel as CancelAction
participant Recording as Recording Session
participant Overlay as Overlay System
Note over Recording,User: During active recording (cancel binding registered)
User->>EventStream: Press Escape (or Fn+Escape)
EventStream->>FnMonitor: Key / FlagsChanged event
FnMonitor->>Shortcut: dispatch_binding_event("cancel", "escape" or "fn+escape", Pressed)
Shortcut->>Cancel: invoke CancelAction.start()
Cancel->>Recording: cancel recording operation
Cancel->>Overlay: hide recording overlay
Cancel->>Shortcut: unregister_dynamic_binding("cancel")
Shortcut-->>Cancel: acknowledge unregistration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
docs/keyboard-shortcuts-plan.md (3)
34-34: Specify language identifier for fenced code blocks.Both ASCII architecture diagrams lack language identifiers in their code fence declarations. This violates the markdownlint MD040 rule and impacts syntax highlighting and accessibility.
Apply these diffs to resolve the issue:
-``` +```text ┌─────────────────────────────────────────────────────────────┐And for line 92:
-``` +```text ┌─────────────────────────────────────────────────────────────┐Also applies to: 92-92
124-129: Clarify integration between macOS fn key monitor and standard shortcuts handler.Phase 1 mentions supplementing
tauri-plugin-global-shortcutwithobjc2-app-kitfor fn key handling. Consider adding a brief note explaining:
- How fn key presses will be distinguished from standard multi-key combinations
- How the system prevents duplicate event handling or missed events
- Whether the CGEventTap listener will have priority over the tauri plugin or run alongside it
This would help implementation and prevent subtle race conditions during development.
74-87: Consider including macOS permission evolution note.The permission matrix and research findings are solid. However, given that macOS permission handling evolves with OS versions, consider adding a note about:
- Minimum macOS version requirements for the proposed approach
- Any known issues with specific macOS versions (Ventura, Sonoma, etc.)
- Whether Input Monitoring permission can be avoided entirely (as implied)
This would help future implementers navigate potential compatibility issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/keyboard-shortcuts-plan.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
docs/keyboard-shortcuts-plan.md
34-34: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
92-92: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (2)
docs/keyboard-shortcuts-plan.md (2)
86-86: Verify objc2 crate availability and maintenance status.The plan relies on using
objc2-app-kitcrates directly (lines 86, 153). Whilst PR cjpais#136 demonstrated this approach works, you should confirm:
- Whether these crates are still actively maintained and have no security issues
- Whether they're suitable for long-term maintenance in Handy
- Whether there are any breaking changes in newer versions
Consider running a verification check on the crates' current status and maintenance frequency.
Also applies to: 151-153
6-7: Well-researched and comprehensive planning document.The analysis is thorough, well-structured, and directly addresses the PR objectives. Key strengths:
- Clear problem statement with four specific, prioritised features
- Documented research with evidence from upstream PRs
- Explicit maintainer feedback integration
- Reasonable phased approach with clear dependencies
- Justified technical decisions with rationale
The document provides a solid foundation for implementation discussions and reviews.
Also applies to: 15-23
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
docs/keyboard-shortcuts-plan.md (3)
327-336: Resolve Open Questions before beginning Phase 1.Four Open Questions are listed but remain unanswered. Three should be decided before implementation starts:
fn key in toggle mode (line 331): Restricting fn to push-to-talk only would require architectural changes if decided post-implementation. Recommend deciding before Phase 2.
Permission prompting strategy (line 333): Whether to prompt on first launch or when user attempts fn binding affects Phase 1 groundwork and user onboarding flow.
Secure Input status visibility (line 336): This affects Phase 1 architecture (does fn monitor need built-in status detection?).
Question 1 (Windows/Linux alternatives) can reasonably be deferred to Phase 2+.
Consider resolving these before Phase 1 to avoid rework and ensure coherent permission/UX strategy.
119-133: Consider runtime Secure Input detection as Phase 1 task.The Secure Input blocking limitation (lines 121–133) is well-documented, and detection via
ioregis proposed. However, the current Implementation Plan does not mention adding runtime detection or user status indication.If the goal is to minimise user confusion when Handy stops working unexpectedly, consider whether Phase 1 (Foundation) should include basic Secure Input detection infrastructure, or whether this is deferred to Phase 5 (Polish) as a stretch goal.
Current phrasing ("consider status indicator" in line 309) suggests this is stretch. If so, recommend explicitly noting in Phase 1 that runtime detection is omitted and only documented as a limitation.
34-34: Add language identifiers to fenced code blocks.Markdown code blocks at lines 34, 96, 180, and 211 lack language specifications, which reduces syntax highlighting and readability. Consider:
- Line 34: Add
```textor```plaintextfor ASCII diagram- Line 96: Change
```to```bashfor the shell command- Line 180: Add
```textfor ASCII diagram- Line 211: Add
```textfor file structure listing### 1. Secure Input Blocking When Secure Input is enabled (password fields, 1Password, Terminal "Secure Keyboard Entry"): - **All event monitoring stops receiving events** - Users will wonder why Handy stopped working - No workaround exists - this is a macOS security feature **Detection possible via:** - ```bash + ```bash ioreg -l -w 0 | grep kCGSSessionSecureInputPIDAlso applies to: 96-96, 180-180, 211-211 </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between d02a1f69651db841faf14a2a1b7e99d6833afb72 and 41d38263a33d212983b0b131e0c4b79fa1ea677f. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `docs/keyboard-shortcuts-plan.md` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>docs/keyboard-shortcuts-plan.md</summary> 34-34: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 96-96: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 180-180: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 211-211: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>🔇 Additional comments (1)</summary><blockquote> <details> <summary>docs/keyboard-shortcuts-plan.md (1)</summary><blockquote> `73-73`: **Removal of `rdev` dependency in Phase 1 is safe to proceed.** Verification confirms that `rdev` has no active usage in the Rust codebase and is a genuine dangling dependency. Additionally, the proposed objc2 ecosystem versions (0.6.x, objc2-app-kit 0.3.x, objc2-foundation 0.3.x, block2 0.6.x) are all currently available and compatible with the current tauri 2.9.1 environment. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
docs/keyboard-shortcuts-plan.md
Outdated
| 1. Remove dangling `rdev` dependency from Cargo.toml | ||
| 2. Add direct dependencies (macOS only): | ||
| ```toml | ||
| [target.'cfg(target_os = "macos")'.dependencies] | ||
| objc2 = "0.6" | ||
| objc2-app-kit = "0.3" | ||
| objc2-foundation = "0.3" | ||
| block2 = "0.6" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify dependency versions and compatibility before Phase 1.
Phase 1 specifies exact versions for the objc2 ecosystem: objc2 0.6, objc2-app-kit 0.3, objc2-foundation 0.3, and block2 0.6. These need verification for:
- Availability on crates.io
- Compatibility with current Tauri/macOS deployment target
- Compatibility with existing transitive dependencies (e.g., from
tauri-nspanel)
Additionally, confirm whether objc2 crates are already available as transitive dependencies (line 72) or if direct additions are required.
🏁 Script executed:
#!/bin/bash
# Check if objc2 ecosystem crates are available and compatible
curl -s https://crates.io/api/v1/crates/objc2 | jq '.versions[] | select(.num == "0.6") | .num'
curl -s https://crates.io/api/v1/crates/objc2-app-kit | jq '.versions[] | select(.num == "0.3") | .num'
curl -s https://crates.io/api/v1/crates/block2 | jq '.versions[] | select(.num == "0.6") | .num'Length of output: 0
🌐 Web query:
What is the latest version of objc2 crate, and is version 0.6 still current as of November 2025?
💡 Result:
The latest released objc2 is 0.6.3 (current as of Nov 22, 2025). Version 0.6.0 (or 0.6) is outdated — the 0.6.x series has newer patch releases (0.6.3). [1][2]
Sources:
[1] docs.rs (objc2 0.6.3).
[2] deps.rs (objc2 0.6.3 metadata).
🌐 Web query:
What is the latest version of objc2-app-kit crate?
💡 Result:
The latest released version is 0.3.2. [1][2]
Sources:
[1] docs.rs — objc2-app-kit (0.3.2).
[2] deps.rs — objc2-app-kit 0.3.2.
I need to verify the remaining dependency versions before finalising the assessment.
Update dependency versions to latest patches in Phase 1 plan.
The specified versions are outdated. objc2-foundation and related crates have newer patch releases available:
- objc2 0.6 (latest 0.6.3)
- objc2-app-kit 0.3 (latest 0.3.2)
- objc2-foundation 0.3 (latest 0.3.1)
- block2 0.6 (latest 0.6.1)
Update lines 269–276 to use the latest patch versions to ensure compatibility and bug fixes.
🤖 Prompt for AI Agents
In docs/keyboard-shortcuts-plan.md around lines 269 to 276, the Phase 1 plan
lists macOS-only crate versions that are outdated; update the dependency entries
to the latest patch releases by changing objc2 to 0.6.3, objc2-app-kit to 0.3.2,
objc2-foundation to 0.3.1, and block2 to 0.6.1 so the Cargo.toml snippets
reflect the current patch versions for compatibility and bug fixes.
dannysmith
force-pushed
the
keyboard-shortcuts
branch
from
41d3826 to
d1300a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
docs/fn-key-and-cancel-implementation.md (7)
10-19: Clarify scope of "minimal UI changes" relative to "DO NOT TOUCH" warning.Lines 10–19 emphasise not touching the existing
HandyShortcut.tsx, yet lines 57–59 propose adding a "Use fn" button. This creates an apparent contradiction. Whilst adding a single button is indeed minimal, this should be explicitly framed as an exception to the "DO NOT TOUCH" rule rather than a contradiction.Consider rewording to clarify: "The existing
HandyShortcut.tsxmust not be restructured or rewritten. A small, isolated 'Use fn' button may be added that callsupdateBinding(id, "fn")without modifying the suspend/resume flow."Also applies to: 57-59
3-3: Minor grammar improvement.The phrase "how to correctly implement" reads more naturally as "how to implement correctly".
58-58: Add missing comma after closing backtick.Change
"Use fn"→"Use fn"with appropriate punctuation.
184-184: Rephrase to avoid awkward noun usage."The suspend → change_binding → resume flow" reads more clearly as "The suspend–change_binding–resume flow" or "The flow: suspend → change_binding → resume". Avoid using "suspend" as a bare noun in this context.
190-190: Remove word repetition."Don't change change_binding behavior" should be "Don't modify
change_bindingbehaviour" or "Don't change thechange_bindingfunction's behaviour".
210-210: Specify language for fenced code block.Line 210 opens a code block without a language identifier. Add the language:
```textor```plaintext.
221-231: Testing checklist is comprehensive but lacks coverage for edge cases.The checklist covers the happy path well, but consider adding:
- Attempt to bind "fn" on non-macOS platforms (should fail validation)
- Attempt to press Escape when recording is not active (should not trigger cancel)
- Multiple rapid Escape presses during recording (should handle gracefully)
- Cancel action during various recording states (initialising, transcribing, error state)
- Accessibility permission denied (fn monitor should fail gracefully)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/fn-key-and-cancel-implementation.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/fn-key-and-cancel-implementation.md
[style] ~3-~3: Try moving the adverb to make the sentence clearer.
Context: ...el Shortcut This document captures how to correctly implement these two features, based on a failed attempt that broke t...
(SPLIT_INFINITIVE)
[uncategorized] ~58-~58: Possible missing comma found.
Context: ...nge)**: - Add a "Use fn" button that calls updateBinding(id, "fn") - That's i...
(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~184-~184: The verb ‘suspend’ does not usually follow articles like ‘The’. Check that ‘suspend’ is spelled correctly; using ‘suspend’ as a noun may be non-standard.
Context: ...## 2. Don't try to "fix" suspend/resume The suspend → change_binding → resume flow has a do...
(A_INFINITIVE)
[duplication] ~190-~190: Possible typo: you repeated a word.
Context: ...sting transcribe binding. ### 4. Don't change change_binding behavior I modified `change_bin...
(ENGLISH_WORD_REPEAT_RULE)
[style] ~194-~194: Consider shortening or rephrasing this to strengthen your wording.
Context: ...preserved. ### 5. Test incrementally I made changes to: - shortcut/mod.rs - shortcut/fn_monito...
(MAKE_CHANGES)
🪛 markdownlint-cli2 (0.18.1)
docs/fn-key-and-cancel-implementation.md
210-210: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (6)
docs/fn-key-and-cancel-implementation.md (6)
63-84: Technical completeness: fn_monitor event handler integration.The code example shows the monitoring logic but does not clarify:
- How is the
app_handlepassed into the closure?- How does
dispatch_binding_event()route "fn" events to the correct action (transcribe)?- Is the handler lifetime managed correctly (stored in
appstate, or global)?Consider adding a note that explains the glue code or links to the actual implementation pattern used elsewhere in the codebase.
110-132: Error handling missing from CancelAction.The
CancelAction::start()spawns a background thread to unregister the binding after a 10 ms delay. However:
- What if
unregister_dynamic_binding()fails?- What if the user presses Escape again before the 10 ms delay completes?
- Should there be a flag to prevent double-cancellation?
Add notes on these edge cases or propose defensive logic.
147-171: Clarify integration points for dynamic binding registration.Lines 147–151 define the public API (
register_dynamic_binding,unregister_dynamic_binding), but the implementation is not shown. Lines 163–171 show where these are called inactions.rs, but:
- What if
register_dynamic_binding()fails during transcription start? Should the transcription abort?- Should failure be logged?
- Is there any cleanup required if
unregister_dynamic_binding()fails?Document the expected error semantics or propose error-handling code patterns.
92-98: Main thread requirement not integrated into fn_monitor spec.The document states (lines 92–98) that the fn monitor "must run on main thread" and shows the pattern
app.run_on_main_thread(move || { ... }). However, the fn_monitor code example (lines 63–84) does not show where this wrapping occurs.Update the fn_monitor code example to show that the entire monitoring setup (lines 63–84) must be wrapped in
app.run_on_main_thread()to avoid confusion during implementation.
39-43: Clarify routing logic for fn binding detection.The proposed routing (line 41–42) delegates to
fn_monitor::register_fn_binding()only ifbinding == "fn". However, it's unclear:
- Is the check case-sensitive? (The helper uses
eq_ignore_ascii_case, line 43, so it should be case-insensitive.)- Should validation occur before or after the routing?
- What happens if someone tries to bind "fn" on non-macOS platforms?
Recommend explicitly stating: "Case-insensitive check on macOS only; on non-macOS, validation should reject 'fn' as invalid." Consider showing the validation logic in the code example.
134-145: Settings schema for dynamic bindings could be more robust.The cancel binding is marked
dynamic: trueto skip registration at startup (line 156–161). However:
- What prevents a user from manually editing the JSON to change
dynamic: truetodynamic: false?- Should the UI hide dynamic bindings from the user?
- Is there any validation to ensure only known dynamic bindings (e.g., "cancel") are registered this way?
Consider documenting the intended user-facing behaviour and any validation guards.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/components/settings/HandyShortcut.tsx (1)
20-23: Keep fn‑binding detection consistent between frontend and backendThe small
isFnBindinghelper is fine, but you now have parallel logic here and insrc-tauri/src/shortcut/mod.rs::is_fn_binding. To avoid future drift if you ever broaden fn semantics (for example to treat"fn+…"specially), consider at least documenting that these must stay in sync, or centralising the canonical string ("fn") in a shared constant.src-tauri/src/shortcut/mod.rs (3)
14-41: Binding routing cleanly separates fn bindings from normal shortcutsThe
is_fn_binding+register_binding/unregister_bindingwrappers give you a neat switch between the fn monitor and the existing global‑shortcut path, and the use ofcfg(target_os = "macos")keeps non‑macOS builds free of fn‑specific code. This should make it much easier to evolve fn handling without touching the rest of the shortcut plumbing.If you ever find yourself cloning
ShortcutBindinga lot, you could have these helpers take&ShortcutBindinginstead and clone only when necessary, but that’s an optimisation rather than a requirement.
655-674: Consider rejectingfnbindings on non‑macOS at validation time
validate_shortcut_stringspecial‑cases"fn"undercfg(target_os = "macos"), which is good. On other platforms though,"fn"is treated as a non‑modifier key (it’s not in themodifierslist), so validation passes, and the user only sees an error later when the string fails to parse as a shortcut in_register_shortcut.It would be clearer to users if
"fn"were rejected up front on non‑macOS, for example by:
- Adding
"fn"to themodifierslist for non‑macOS and explicitly erroring on modifier‑only combos, or- Adding a non‑macOS
if is_fn_binding(raw) { return Err("fn key is only supported on macOS".into()); }branch.That way, people editing JSON or migrating configs don’t get a confusing “cannot parse shortcut” error.
701-747: Unified dispatch helper looks correct; watch settings read cost if this becomes hotRefactoring the registration closure to delegate to
dispatch_binding_eventis a nice simplification and ensures fn‑monitor and global‑shortcut events share identical behaviour (push‑to‑talk vs toggle, sharedManagedToggleState, andACTION_MAPlookup).
dispatch_binding_eventcallsget_settings(app)on every key event, which (persettings.rs) re‑deserialises the stored settings. That’s probably fine for current usage, but if shortcut activity becomes very frequent you might eventually want to cache just thepush_to_talkflag in app state instead of reloading the whole settings object on every press/release.Also applies to: 779-787
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
src-tauri/Cargo.toml(1 hunks)src-tauri/src/shortcut/fn_monitor.rs(1 hunks)src-tauri/src/shortcut/mod.rs(8 hunks)src/components/settings/HandyShortcut.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/components/settings/HandyShortcut.tsx (2)
src-tauri/src/shortcut/mod.rs (2)
binding(760-760)binding(799-799)src/lib/utils/keyboard.ts (1)
formatKeyCombination(161-168)
src-tauri/src/shortcut/fn_monitor.rs (1)
src-tauri/src/shortcut/mod.rs (4)
app(722-722)binding(760-760)binding(799-799)dispatch_binding_event(703-747)
src-tauri/src/shortcut/mod.rs (2)
src-tauri/src/shortcut/fn_monitor.rs (2)
register_fn_binding(107-133)unregister_fn_binding(136-157)src-tauri/src/settings.rs (2)
load_or_create_app_settings(427-455)get_settings(457-473)
🔇 Additional comments (3)
src-tauri/Cargo.toml (1)
87-92: macOS-only Objective‑C bridge dependencies look appropriateThe added objc2 / AppKit / Foundation / block2 dependencies under the macOS target cleanly scope the new fn‑monitor functionality to macOS without affecting other platforms. This wiring matches the new fn_monitor module’s usage and keeps the global dependency surface reasonable. Please just confirm these exact versions and feature flags are what you’ve tested with across your supported macOS versions.
src-tauri/src/shortcut/fn_monitor.rs (1)
1-296: fn/Globe monitor implementation and state handling look robustThe fn monitor module is well‑structured: you gate initialisation on Accessibility permission, install the global NSEvent monitor on the main thread with a clear handshake, and keep runtime state in a shared
FnMonitorStatewhile ensuring the mutex is released before dispatching actions. Thefn_pressededge detection and cloning ofFnBindingEntrys also avoid redundant dispatches and lock contention. I don’t see any correctness issues in the way you start/stop monitoring or route fn events intodispatch_binding_event.src/components/settings/HandyShortcut.tsx (1)
293-346: Review comment is incorrect—the code is actually correct for Tauri v2This codebase uses Tauri v2 (
@tauri-apps/plugin-os ~2.3.2), wheretype()is a synchronous function returningOsTypedirectly. The code at line 46 (const detectedType = type();) is therefore correct and requires noawait.The review comment incorrectly assumes Tauri v1 API, where
type()is asynchronous and returns aPromise. In Tauri v2, the function is synchronous, so the switch statement will match properly,osTypewill be set correctly, and macOS users will see the "Use fn" button as intended.Likely an incorrect or invalid review comment.
* origin/main: cf Add wayland paste support using wtype or dotool (cjpais#376) feat(audio): async play test sound (cjpais#375) feat: Add disabled option for pasting. (cjpais#364) WIP: Add tauri-specta for generating TS bindings (cjpais#322)
dannysmith
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src-tauri/src/settings.rs (1)
76-88: Existing users will miss the new “cancel” binding unless settings are migrated
get_default_settingscorrectly initialises both"transcribe"(dynamic: false) and"cancel"(dynamic: true, Escape) bindings, butload_or_create_app_settingssimply deserialises whatever is insettings_store.json. For users who already have a storedbindingsmap without"cancel",register_dynamic_binding(app, "cancel")will fail with “Dynamic binding 'cancel' not found in settings”, so Escape-to-cancel will never actually register for them.Consider adding a lightweight migration step after loading settings to ensure new default bindings exist, for example:
pub fn load_or_create_app_settings(app: &AppHandle) -> AppSettings { - let settings = if let Some(settings_value) = store.get("settings") { + let mut settings = if let Some(settings_value) = store.get("settings") { // Parse the entire settings object match serde_json::from_value::<AppSettings>(settings_value) { Ok(settings) => { debug!("Found existing settings: {:?}", settings); settings } Err(e) => { warn!("Failed to parse settings: {}", e); // Fall back to default settings if parsing fails let default_settings = get_default_settings(); store.set("settings", serde_json::to_value(&default_settings).unwrap()); default_settings } } } else { let default_settings = get_default_settings(); store.set("settings", serde_json::to_value(&default_settings).unwrap()); default_settings }; + + // Migration: ensure the new "cancel" binding exists for existing users + if !settings.bindings.contains_key("cancel") { + let mut defaults = get_default_settings(); + if let Some(cancel) = defaults.bindings.remove("cancel") { + settings.bindings.insert(cancel.id.clone(), cancel); + store.set("settings", serde_json::to_value(&settings).unwrap()); + } + } - settings + settings }This way, the Escape-to-cancel feature is available for both new and existing installations without requiring users to reset their settings.
Also applies to: 429-451
src/components/settings/HandyShortcut.tsx (1)
283-291: Avoid using the first binding entry as the “primary” now that dynamic bindings existWith the new
"cancel"binding added inAppSettings.bindings(and markeddynamic: true), relying onObject.values(bindings)[0]/Object.keys(bindings)[0]can end up selecting the cancel binding instead of the main “transcribe” shortcut, asHashMapserialisation order is not guaranteed. That would both expose a dynamic binding in the UI (contrary to the doc comment) and cause the “Use fn” button to target the wrong binding.It would be safer to pick a non-dynamic binding explicitly, e.g.:
- const primaryBinding = Object.values(bindings)[0]; - const primaryId = Object.keys(bindings)[0]; + const bindingIds = Object.keys(bindings); + const primaryId = + bindingIds.find((id) => !bindings[id]?.dynamic) ?? bindingIds[0]; + const primaryBinding = bindings[primaryId];This keeps the component focused on the user-editable shortcut even as more dynamic bindings are added in future.
Also applies to: 293-339
♻️ Duplicate comments (1)
src-tauri/src/shortcut/mod.rs (1)
64-107: Dynamic binding helpers assume settings contain the binding (see migration note in settings.rs)
register_dynamic_binding/unregister_dynamic_bindingcorrectly enforcebinding.dynamicand route throughregister_binding/unregister_binding, but they depend on the binding id being present insettings.bindings. For existing users whose stored settings don’t yet contain the new"cancel"entry, this will yield a"Dynamic binding 'cancel' not found in settings"error when you try to enable Escape-to-cancel (though you already swallow it at call sites).The underlying fix is to ensure new default bindings are injected into existing settings on load; see the migration suggestion in
src-tauri/src/settings.rs::load_or_create_app_settings.
🧹 Nitpick comments (2)
docs/fn-key-and-cancel-implementation.md (1)
90-100: Add a language to the file-structure code fenceThe implementation notes look accurate and very helpful. To satisfy markdownlint (MD040) and improve rendering, consider adding a language to the file-tree code block (for example
```textbeforesrc-tauri/src/).src-tauri/src/actions.rs (1)
265-272: Dynamic cancel action wiring works, but its lifetime in toggle mode is slightly broader than describedThe
TranscribeAction/CancelActionwiring looks coherent:
- start: register dynamic
"cancel"viaregister_dynamic_bindingon the main thread;- stop: unregister it again; and
CancelAction::startdelegates tocancel_current_operationwithout touching registration, avoiding the global_shortcut internal lock issue you noted.One nuance in toggle mode: because
CancelAction::startresets all toggle states and cancels recording without ever triggeringTranscribeAction::stopfor that session, the"cancel"shortcut remains registered until the next full transcribe start/stop cycle. Functionally this mostly just means Escape stays globally bound (calling a largely idempotent cancel path) even when nothing is recording, which is slightly looser than the “only active while recording” wording in the docs.If you want the implementation to more closely match the documented lifetime in toggle mode as well, you might consider scheduling an
unregister_dynamic_binding("cancel")viarun_on_main_threadfrom outside the shortcut callback path (e.g. aftercancel_current_operationcompletes), while still respecting the deadlock constraint.Also applies to: 283-290, 425-447, 472-483
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/fn-key-and-cancel-implementation.md(1 hunks)src-tauri/src/actions.rs(4 hunks)src-tauri/src/settings.rs(2 hunks)src-tauri/src/shortcut/fn_monitor.rs(1 hunks)src-tauri/src/shortcut/mod.rs(7 hunks)src-tauri/src/utils.rs(1 hunks)src/bindings.ts(1 hunks)src/components/settings/HandyShortcut.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src-tauri/src/shortcut/fn_monitor.rs
🧰 Additional context used
🧬 Code graph analysis (4)
src-tauri/src/utils.rs (5)
src-tauri/src/actions.rs (5)
app(214-214)app(221-221)app(292-292)app(293-293)app(294-294)src-tauri/src/shortcut/mod.rs (1)
app(819-819)src-tauri/src/commands/audio.rs (2)
app(51-51)app(102-102)src-tauri/src/tray.rs (3)
app(58-58)app(134-134)change_tray_icon(57-74)src-tauri/src/overlay.rs (1)
hide_recording_overlay(234-247)
src/components/settings/HandyShortcut.tsx (2)
src-tauri/src/shortcut/mod.rs (2)
binding(856-856)binding(899-899)src/lib/utils/keyboard.ts (1)
formatKeyCombination(161-168)
src-tauri/src/actions.rs (2)
src-tauri/src/shortcut/mod.rs (3)
app(819-819)register_dynamic_binding(72-94)unregister_dynamic_binding(97-107)src-tauri/src/utils.rs (3)
app(25-25)app(34-34)cancel_current_operation(18-48)
src-tauri/src/shortcut/mod.rs (4)
src-tauri/src/utils.rs (2)
app(25-25)app(34-34)src/bindings.ts (1)
ShortcutBinding(613-619)src-tauri/src/shortcut/fn_monitor.rs (2)
register_fn_binding(105-131)unregister_fn_binding(134-155)src-tauri/src/settings.rs (2)
load_or_create_app_settings(512-540)get_settings(542-558)
🪛 markdownlint-cli2 (0.18.1)
docs/fn-key-and-cancel-implementation.md
92-92: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (5)
src/bindings.ts (1)
613-619: Dynamic binding flag shape looks soundThe
dynamic?: booleanflag and its documentation align with the RustShortcutBinding(#[serde(default)] pub dynamic: bool), so older settings remain valid (field simply absent / false) while still allowing dynamic bindings to be modelled explicitly. No changes needed here.src-tauri/src/utils.rs (1)
14-47: Cancellation flow correctly discards recording and resets UIThe refactored
cancel_current_operationcleanly cancels audio, removes mute, resets all toggle states without callingaction.stop(), and normalises overlay/tray state to Idle. This matches the “cancel without transcribing” requirement and avoids lock re-entrancy issues given the updateddispatch_binding_eventlogic.src-tauri/src/shortcut/mod.rs (2)
789-847: Centralised dispatch logic is robust and avoids lock re-entrancy issues
dispatch_binding_eventunifies handling of all shortcuts (including fn-monitor-driven ones) and:
- respects
push_to_talkvs toggle mode;- uses
ManagedToggleStateonly in toggle mode; and- crucially, releases the toggle-state mutex before invoking
action.start/stop, which is important now thatCancelAction’scancel_current_operationalso needs to acquire that lock.This significantly reduces the chance of deadlocks and keeps behaviour consistent across global_shortcut and fn-monitor paths.
18-21: fn-specific routing and validation look correctThe
is_fn_bindinghelper plusregister_binding/unregister_bindingrouting, combined with the"fn"special case invalidate_shortcut_string, give a clear macOS-only path for fn/globe bindings while keeping the existing validation intact for all other platforms._register_shortcutdelegating todispatch_binding_eventin its callback means fn-monitor and regular shortcuts share the same action semantics. This all looks coherent.Also applies to: 737-760, 849-895
src/components/settings/HandyShortcut.tsx (1)
42-71: The review comment is factually incorrect —type()is synchronous, not async.According to the official Tauri v2 documentation,
type()from@tauri-apps/plugin-oshas the signaturefunction type(): OsTypeand returns a string directly, not a Promise. The code at line 46 correctly callsconst detectedType = type();withoutawait, and the switch statement will properly evaluate the OS type.osTypewill be set correctly based on the actual operating system, and the macOS-specific UI will activate as intended.The suggested diff to add
awaitis unnecessary and incorrect.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src-tauri/src/shortcut/mod.rs (1)
64-107: Verify API visibility for dynamic binding functions.The idempotent registration pattern (unregister-then-register) is well-designed for avoiding conflicts and deadlocks. However, these functions are marked
pub, making them part of the crate's public API.If these functions are only intended for internal use (called from within the same crate), consider restricting visibility to
pub(crate)to prevent external misuse.-pub fn register_dynamic_binding(app: &AppHandle, binding_id: &str) -> Result<(), String> { +pub(crate) fn register_dynamic_binding(app: &AppHandle, binding_id: &str) -> Result<(), String> {-pub fn unregister_dynamic_binding(app: &AppHandle, binding_id: &str) -> Result<(), String> { +pub(crate) fn unregister_dynamic_binding(app: &AppHandle, binding_id: &str) -> Result<(), String> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src-tauri/src/shortcut/mod.rs(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src-tauri/src/shortcut/mod.rs (3)
src-tauri/src/actions.rs (5)
app(214-214)app(221-221)app(292-292)app(293-293)app(294-294)src-tauri/src/shortcut/fn_monitor.rs (2)
register_fn_binding(105-131)unregister_fn_binding(134-155)src-tauri/src/settings.rs (2)
load_or_create_app_settings(512-540)get_settings(542-558)
🔇 Additional comments (7)
src-tauri/src/shortcut/mod.rs (7)
18-46: LGTM! Clean routing abstraction for platform-specific bindings.The helper functions correctly route fn-key bindings to the macOS-specific monitor and delegate other shortcuts to the standard registration path. The platform-conditional compilation is appropriate.
48-62: LGTM! Dynamic binding lifecycle handled correctly.Skipping dynamic bindings during initialization is appropriate, as they're intended to be registered/unregistered at runtime based on application state.
116-180: LGTM! Consistent use of routing helpers.The function correctly uses the new
register_bindingandunregister_bindinghelpers, and appropriately downgrades the log level for expected unregister failures.
737-762: LGTM! Platform-appropriate validation for fn key.The special case handling for the macOS fn key is correctly implemented with appropriate platform guards and clear error messages for unsupported platforms.
764-789: LGTM! Routing helpers applied consistently.Both functions correctly use the new routing helpers to handle platform-specific binding types.
791-849: Well-designed dispatch with explicit deadlock avoidance.The central dispatch function correctly handles both PTT and toggle modes. The pattern of acquiring the lock, determining the action, releasing the lock, and then calling the action (lines 816-840) effectively prevents deadlocks.
Note: Line 824 uses
.expect()on lock acquisition, which will panic if the lock is poisoned. This is typically acceptable for critical state that cannot be recovered, but ensure this aligns with the application's error handling strategy.
851-919: LGTM! Centralized dispatch improves maintainability.The refactoring to delegate to
dispatch_binding_eventeliminates duplication and ensures consistent handling of both PTT and toggle modes across all binding types. The improved error message formatting and debug logging are helpful additions.
Resolved conflicts: - actions.rs: Keep dynamic binding approach, add recording_started check from main - shortcut/mod.rs: Keep dispatch_binding_event, add Linux guards from PR cjpais#392 - utils.rs: Keep proper cancel_current_operation ordering (cancel first) - HandyShortcut.tsx: Keep fn key UI support - settings.rs: Remove duplicate cancel binding from main's PR cjpais#224 Added Linux guards to register_dynamic_binding() and unregister_dynamic_binding() to match PR cjpais#392's stability fix for Linux. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src-tauri/src/shortcut/mod.rs (1)
176-189: Consider generalising dynamic binding handling.The special case for the "cancel" binding works, but it hardcodes the binding ID. Consider generalising this to check the
dynamicflag instead:- // If this is the cancel binding, just update the settings and return - // It's managed dynamically, so we don't register/unregister here - if id == "cancel" { + // If this is a dynamic binding, just update the settings and return + // Dynamic bindings are managed at runtime, so we don't register/unregister here + if binding_to_modify.dynamic { if let Some(mut b) = settings.bindings.get(&id).cloned() { b.current_binding = binding; settings.bindings.insert(id.clone(), b.clone());This makes the code more maintainable if additional dynamic bindings are added in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
docs/fn-key-and-cancel-implementation.md(1 hunks)src-tauri/Cargo.toml(1 hunks)src-tauri/src/actions.rs(3 hunks)src-tauri/src/settings.rs(2 hunks)src-tauri/src/shortcut/mod.rs(6 hunks)src-tauri/src/utils.rs(1 hunks)src/bindings.ts(1 hunks)src/components/settings/HandyShortcut.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/bindings.ts
- src/components/settings/HandyShortcut.tsx
- src-tauri/src/actions.rs
- src-tauri/Cargo.toml
🧰 Additional context used
🧬 Code graph analysis (1)
src-tauri/src/utils.rs (3)
src-tauri/src/tray.rs (3)
app(59-59)app(137-137)change_tray_icon(58-75)src-tauri/src/commands/audio.rs (2)
app(51-51)app(102-102)src-tauri/src/overlay.rs (1)
hide_recording_overlay(269-282)
🪛 GitHub Actions: prettier
docs/fn-key-and-cancel-implementation.md
[warning] 1-1: Prettier detected code style issues. Run 'prettier --write' to fix.
🪛 markdownlint-cli2 (0.18.1)
docs/fn-key-and-cancel-implementation.md
92-92: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (11)
docs/fn-key-and-cancel-implementation.md (1)
1-133: Excellent documentation.The documentation is comprehensive and well-structured. It clearly explains the implementation approach, file structure, design decisions, and testing requirements. The references to relevant PRs and Linux limitations are particularly helpful.
Note: Please run Prettier to address the formatting issue flagged in the pipeline:
prettier --write docs/fn-key-and-cancel-implementation.mdsrc-tauri/src/utils.rs (1)
14-48: Well-structured cancellation flow.The cancellation logic is correctly sequenced:
- Audio recording is cancelled first (preventing data loss)
- Mute is removed (restoring audio state)
- Toggle states are reset without triggering transcription
- UI state is cleaned up (overlay and tray icon)
The documentation clearly explains why
action.stop()is not called, preventing unintended transcription. The explicit ordering comments are helpful for future maintainers.src-tauri/src/settings.rs (2)
83-87: Clean addition of dynamic binding support.The
dynamicfield is well-documented and uses#[serde(default)]to ensure backward compatibility with existing settings files. This allows for runtime registration/unregistration of bindings like the cancel shortcut.
447-457: The concern about case sensitivity is not substantiated by the codebase or available documentation.The "Escape" key capitalization is consistent with the tauri-plugin-global-shortcut library's key naming conventions (Code::Escape enum variant). The validation function only normalizes modifier keys (ctrl, shift, alt, etc.) to lowercase; it does not validate or transform the key name itself. Web documentation for tauri-plugin-global-shortcut shows the library accepts multiple forms (both "escape" and "esc"), suggesting case-insensitive parsing. No errors, warnings, or compatibility issues related to this capitalization exist in the codebase. The dynamic binding is correctly marked and only registered during recording.
src-tauri/src/shortcut/mod.rs (7)
15-46: Clean routing for macOS fn key support.The conditional compilation and routing logic for macOS fn key bindings is well-implemented:
- Case-insensitive matching for "fn" bindings
- Proper use of
#[cfg(target_os = "macos")]guards- Clear separation between fn_monitor and regular shortcut paths
48-70: Correct handling of dynamic bindings during initialisation.The updated
init_shortcutscorrectly skips dynamic bindings (like cancel), which are registered at runtime when needed. The debug logging provides good observability.
83-119: Well-designed idempotent registration for dynamic bindings.The
register_dynamic_bindingfunction handles several important concerns:
- Idempotent behavior (unregister before register) prevents deadlock when called from shortcut callbacks
- Linux disabling is documented and intentional (PR cjpais#392)
- Proper validation that the binding is marked as dynamic
- Good error messages for troubleshooting
The design avoids the need for callers to explicitly unregister, simplifying the API and preventing common pitfalls.
125-145: LGTM on unregister_dynamic_binding.The function mirrors
register_dynamic_bindingwith consistent Linux disabling and error handling.
807-832: Correct validation for macOS-specific fn key.The
validate_shortcut_stringfunction properly validates that the "fn" binding is only used on macOS, with a clear error message for other platforms.
861-919: Excellent deadlock avoidance in dispatch logic.The
dispatch_binding_eventfunction handles a critical concurrency concern correctly:In toggle mode (lines 886-903), the function:
- Acquires the toggle state lock
- Determines which action to perform
- Releases the lock before calling the action
This is essential because actions like
cancel_current_operationneed to acquire the same lock. The explicit comment explaining this design decision is valuable for maintainers.The unified dispatch point for both global-shortcut and fn_monitor paths is clean architecture.
921-968: Clean refactoring to use centralised dispatch.The refactoring of
_register_shortcutto use the centraliseddispatch_binding_eventfunction eliminates code duplication and ensures consistent behavior across both shortcut systems.
| ``` | ||
| src-tauri/src/ | ||
| ├── shortcut/ | ||
| │ ├── mod.rs # Shortcut logic, routing, dispatch_binding_event | ||
| │ └── fn_monitor.rs # macOS-only fn key monitoring | ||
| ├── actions.rs # CancelAction, TranscribeAction with cancel registration | ||
| ├── settings.rs # ShortcutBinding.dynamic field, cancel binding | ||
| └── utils.rs # cancel_current_operation() | ||
| ``` |
There was a problem hiding this comment.
Add language identifier to fenced code block.
The fenced code block at line 92 is missing a language identifier, which reduces readability and prevents syntax highlighting.
Apply this diff to add the language identifier:
-```
+```text
src-tauri/src/
├── shortcut/
│ ├── mod.rs # Shortcut logic, routing, dispatch_binding_event🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
92-92: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In docs/fn-key-and-cancel-implementation.md around lines 92 to 100, the fenced
code block lacks a language identifier; update the opening fence from ``` to
```text so the block becomes ```text ... ``` (i.e., add the language identifier
"text" to the existing fenced code block) to enable proper syntax highlighting
and readability.
Summary
This PR documents analysis and proposes an implementation plan for improving Handy's keyboard shortcut system.
See the full document: docs/keyboard-shortcuts-plan.md
Problem Statement
Handy's keyboard shortcut system needs improvement to support:
Key Findings
objc2-app-kitcratesProposed Approach
objc2-app-kit(supplement, not replacetauri-plugin-global-shortcut)Test Plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Settings
✏️ Tip: You can customize this high-level summary in your review settings.