Merge pull request #71 from cjpais/binding-handling

disallow invalid bindings from frontend

package.json

@@ -25,6 +25,7 @@
     "@tauri-apps/plugin-upload": "~2",
     "react": "^18.3.1",
     "react-dom": "^18.3.1",
+    "sonner": "^2.0.7",
     "tailwindcss": "^4.0.2",
     "tauri-plugin-macos-permissions-api": "^2.0.4",
     "zod": "^3.24.4"

src-tauri/src/lib.rs

@@ -183,6 +183,8 @@ pub fn run() {
             shortcut::change_selected_language_setting,
             shortcut::change_show_overlay_setting,
             shortcut::change_debug_mode_setting,
+            shortcut::suspend_binding,
+            shortcut::resume_binding,
             trigger_update_check,
             commands::cancel_operation,
             commands::get_app_dir_path,

src-tauri/src/shortcut.rs

@@ -15,7 +15,7 @@ pub fn init_shortcuts(app: &App) {
     for (_id, binding) in settings.bindings {
         // Pass app.handle() which is &AppHandle
         if let Err(e) = _register_shortcut(app.handle(), binding) {
-            eprintln!("Failed to register shortcut {}: {}", _id, e);
+            eprintln!("Failed to register shortcut {} during init: {}", _id, e);
         }
     }
 }
@@ -39,21 +39,26 @@ pub fn change_binding(
     let binding_to_modify = match settings.bindings.get(&id) {
         Some(binding) => binding.clone(),
         None => {
+            let error_msg = format!("Binding with id '{}' not found", id);
+            eprintln!("change_binding error: {}", error_msg);
             return Ok(BindingResponse {
                 success: false,
                 binding: None,
-                error: Some(format!("Binding with id '{}' not found", id)),
+                error: Some(error_msg),
-            })
+            });
         }
     };
 
     // Unregister the existing binding
     if let Err(e) = _unregister_shortcut(&app, binding_to_modify.clone()) {
-        return Ok(BindingResponse {
+        let error_msg = format!("Failed to unregister shortcut: {}", e);
-            success: false,
+        eprintln!("change_binding error: {}", error_msg);
-            binding: None,
+    }
-            error: Some(format!("Failed to unregister shortcut: {}", e)),
+
-        });
+    // Validate the new shortcut before we touch the current registration
+    if let Err(e) = validate_shortcut_string(&binding) {
+        eprintln!("change_binding validation error: {}", e);
+        return Err(e);
     }
 
     // Create an updated binding
@@ -62,10 +67,12 @@ pub fn change_binding(
 
     // Register the new binding
     if let Err(e) = _register_shortcut(&app, updated_binding.clone()) {
+        let error_msg = format!("Failed to register shortcut: {}", e);
+        eprintln!("change_binding error: {}", error_msg);
         return Ok(BindingResponse {
             success: false,
             binding: None,
-            error: Some(format!("Failed to register shortcut: {}", e)),
+            error: Some(error_msg),
         });
     }
 
@@ -143,13 +150,79 @@ pub fn change_debug_mode_setting(app: AppHandle, enabled: bool) -> Result<(), St
     Ok(())
 }
 
+/// Determine whether a shortcut string contains at least one non-modifier key.
+/// We allow single non-modifier keys (e.g. "f5" or "space") but disallow
+/// modifier-only combos (e.g. "ctrl" or "ctrl+shift").
+fn validate_shortcut_string(raw: &str) -> Result<(), String> {
+    let modifiers = [
+        "ctrl", "control", "shift", "alt", "option", "meta", "command", "cmd", "super", "win",
+        "windows",
+    ];
+    let has_non_modifier = raw
+        .split('+')
+        .any(|part| !modifiers.contains(&part.trim().to_lowercase().as_str()));
+    if has_non_modifier {
+        Ok(())
+    } else {
+        Err("Shortcut must contain at least one non-modifier key".into())
+    }
+}
+
+/// Temporarily unregister a binding while the user is editing it in the UI.
+/// This avoids firing the action while keys are being recorded.
+#[tauri::command]
+pub fn suspend_binding(app: AppHandle, id: String) -> Result<(), String> {
+    if let Some(b) = settings::get_bindings(&app).get(&id).cloned() {
+        if let Err(e) = _unregister_shortcut(&app, b) {
+            eprintln!("suspend_binding error for id '{}': {}", id, e);
+            return Err(e);
+        }
+    }
+    Ok(())
+}
+
+/// Re-register the binding after the user has finished editing.
+#[tauri::command]
+pub fn resume_binding(app: AppHandle, id: String) -> Result<(), String> {
+    if let Some(b) = settings::get_bindings(&app).get(&id).cloned() {
+        if let Err(e) = _register_shortcut(&app, b) {
+            eprintln!("resume_binding error for id '{}': {}", id, e);
+            return Err(e);
+        }
+    }
+    Ok(())
+}
+
 fn _register_shortcut(app: &AppHandle, binding: ShortcutBinding) -> Result<(), String> {
+    // Validate human-level rules first
+    if let Err(e) = validate_shortcut_string(&binding.current_binding) {
+        eprintln!(
+            "_register_shortcut validation error for binding '{}': {}",
+            binding.current_binding, e
+        );
+        return Err(e);
+    }
+
     // Parse shortcut and return error if it fails
     let shortcut = match binding.current_binding.parse::<Shortcut>() {
         Ok(s) => s,
-        Err(e) => return Err(format!("Failed to parse shortcut: {}", e)),
+        Err(e) => {
+            let error_msg = format!(
+                "Failed to parse shortcut '{}': {}",
+                binding.current_binding, e
+            );
+            eprintln!("_register_shortcut parse error: {}", error_msg);
+            return Err(error_msg);
+        }
     };
 
+    // Prevent duplicate registrations that would silently shadow one another
+    if app.global_shortcut().is_registered(shortcut) {
+        let error_msg = format!("Shortcut '{}' is already in use", binding.current_binding);
+        eprintln!("_register_shortcut duplicate error: {}", error_msg);
+        return Err(error_msg);
+    }
+
     // Clone binding.id for use in the closure
     let binding_id_for_closure = binding.id.clone();
 
@@ -197,7 +270,11 @@ fn _register_shortcut(app: &AppHandle, binding: ShortcutBinding) -> Result<(), S
                 }
             }
         })
-        .map_err(|e| format!("Couldn't register shortcut: {}", e))?;
+        .map_err(|e| {
+            let error_msg = format!("Couldn't register shortcut '{}': {}", binding.current_binding, e);
+            eprintln!("_register_shortcut registration error: {}", error_msg);
+            error_msg
+        })?;
 
     Ok(())
 }
@@ -205,12 +282,24 @@ fn _register_shortcut(app: &AppHandle, binding: ShortcutBinding) -> Result<(), S
 fn _unregister_shortcut(app: &AppHandle, binding: ShortcutBinding) -> Result<(), String> {
     let shortcut = match binding.current_binding.parse::<Shortcut>() {
         Ok(s) => s,
-        Err(e) => return Err(format!("Failed to parse shortcut: {}", e)),
+        Err(e) => {
+            let error_msg = format!(
+                "Failed to parse shortcut '{}' for unregistration: {}",
+                binding.current_binding, e
+            );
+            eprintln!("_unregister_shortcut parse error: {}", error_msg);
+            return Err(error_msg);
+        }
     };
 
-    app.global_shortcut()
+    app.global_shortcut().unregister(shortcut).map_err(|e| {
-        .unregister(shortcut)
+        let error_msg = format!(
-        .map_err(|e| format!("Failed to unregister shortcut: {}", e))?;
+            "Failed to unregister shortcut '{}': {}",
+            binding.current_binding, e
+        );
+        eprintln!("_unregister_shortcut error: {}", error_msg);
+        error_msg
+    })?;
 
     Ok(())
 }

src/App.tsx

@@ -6,6 +6,7 @@ import Footer from "./components/footer";
 import HandyTextLogo from "./components/icons/HandyTextLogo";
 import Onboarding from "./components/onboarding";
 import { Settings } from "./components/settings/Settings";
+import { Toaster } from "sonner";
 
 function App() {
   const [showOnboarding, setShowOnboarding] = useState<boolean | null>(null);
@@ -43,6 +44,7 @@ function App() {
 
   return (
     <div className="min-h-screen flex flex-col w-full">
+      <Toaster />
       <div className="flex flex-col items-center pt-6 gap-8 px-4 flex-1">
         <HandyTextLogo width={240} />
         <AccessibilityPermissions />

src/components/settings/HandyShortcut.tsx

@@ -1,10 +1,17 @@
 import React, { useEffect, useState, useRef } from "react";
 import { BindingResponseSchema, ShortcutBindingsMap } from "../../lib/types";
 import { type } from "@tauri-apps/plugin-os";
-import { getKeyName } from "../../lib/utils/keyboard";
+import {
+  getKeyName,
+  formatKeyCombination,
+  normalizeKey,
+  type OSType,
+} from "../../lib/utils/keyboard";
 import ResetIcon from "../icons/ResetIcon";
 import { SettingContainer } from "../ui/SettingContainer";
 import { useSettings } from "../../hooks/useSettings";
+import { invoke } from "@tauri-apps/api/core";
+import { toast } from "sonner";
 
 interface HandyShortcutProps {
   descriptionMode?: "inline" | "tooltip";
@@ -23,57 +30,42 @@ export const HandyShortcut: React.FC<HandyShortcutProps> = ({
     null,
   );
   const [originalBinding, setOriginalBinding] = useState<string>("");
-  const [isMacOS, setIsMacOS] = useState<boolean>(false);
+  const [osType, setOsType] = useState<OSType>("unknown");
   const shortcutRefs = useRef<Map<string, HTMLDivElement | null>>(new Map());
 
   const bindings = getSetting("bindings") || {};
 
-  // Check if running on macOS
+  // Detect and store OS type
   useEffect(() => {
-    const checkOsType = async () => {
+    const detectOsType = async () => {
       try {
-        const osType = await type();
+        const detectedType = type();
-        setIsMacOS(osType === "macos");
+        let normalizedType: OSType;
+
+        switch (detectedType) {
+          case "macos":
+            normalizedType = "macos";
+            break;
+          case "windows":
+            normalizedType = "windows";
+            break;
+          case "linux":
+            normalizedType = "linux";
+            break;
+          default:
+            normalizedType = "unknown";
+        }
+
+        setOsType(normalizedType);
       } catch (error) {
         console.error("Error detecting OS type:", error);
-        setIsMacOS(false);
+        setOsType("unknown");
       }
     };
 
-    checkOsType();
+    detectOsType();
   }, []);
 
-  // Normalize modifier keys (unify left/right variants)
-  const normalizeKey = (key: string): string => {
-    // Handle left/right variants of modifier keys
-    if (key.startsWith("left ") || key.startsWith("right ")) {
-      const parts = key.split(" ");
-      if (parts.length === 2) {
-        // Return just the modifier name without left/right prefix
-        return parts[1];
-      }
-    }
-    return key;
-  };
-
-  // Format keys for macOS display
-  const formatMacOSKeys = (key: string): string => {
-    if (!isMacOS) return key; // Only format for macOS
-
-    const keyMap: Record<string, string> = {
-      alt: "option",
-    };
-
-    return keyMap[key.toLowerCase()] || key;
-  };
-
-  // Format a key combination for display
-  const formatKeyCombination = (combination: string): string => {
-    if (!isMacOS) return combination; // Only format for macOS
-
-    return combination.split("+").map(formatMacOSKeys).join(" + ");
-  };
-
   useEffect(() => {
     // Only add event listeners when we're in editing mode
     if (editingShortcutId === null) return;
@@ -82,10 +74,34 @@ export const HandyShortcut: React.FC<HandyShortcutProps> = ({
 
     // Keyboard event listeners
     const handleKeyDown = async (e: KeyboardEvent) => {
+      if (e.repeat) return; // ignore auto-repeat
+      if (e.key === "Escape") {
+        // Cancel recording and restore original binding
+        if (editingShortcutId && originalBinding) {
+          try {
+            await updateBinding(editingShortcutId, originalBinding);
+            await invoke("resume_binding", { id: editingShortcutId }).catch(
+              console.error,
+            );
+          } catch (error) {
+            console.error("Failed to restore original binding:", error);
+            toast.error("Failed to restore original shortcut");
+          }
+        } else if (editingShortcutId) {
+          await invoke("resume_binding", { id: editingShortcutId }).catch(
+            console.error,
+          );
+        }
+        setEditingShortcutId(null);
+        setKeyPressed([]);
+        setRecordedKeys([]);
+        setOriginalBinding("");
+        return;
+      }
       e.preventDefault();
 
-      // Get the key and normalize it (unify left/right modifiers)
+      // Get the key with OS-specific naming and normalize it
-      const rawKey = getKeyName(e);
+      const rawKey = getKeyName(e, osType);
       const key = normalizeKey(rawKey);
 
       console.log("You pressed", rawKey, "normalized to", key);
@@ -102,8 +118,8 @@ export const HandyShortcut: React.FC<HandyShortcutProps> = ({
     const handleKeyUp = async (e: KeyboardEvent) => {
       e.preventDefault();
 
-      // Get the key and normalize it
+      // Get the key with OS-specific naming and normalize it
-      const rawKey = getKeyName(e);
+      const rawKey = getKeyName(e, osType);
       const key = normalizeKey(rawKey);
 
       // Remove from currently pressed keys
@@ -118,8 +134,26 @@ export const HandyShortcut: React.FC<HandyShortcutProps> = ({
         if (editingShortcutId && bindings[editingShortcutId]) {
           try {
             await updateBinding(editingShortcutId, newShortcut);
+            // Re-register the shortcut now that recording is finished
+            await invoke("resume_binding", { id: editingShortcutId }).catch(
+              console.error,
+            );
           } catch (error) {
             console.error("Failed to change binding:", error);
+            toast.error(`Failed to set shortcut: ${error}`);
+
+            // Reset to original binding on error
+            if (originalBinding) {
+              try {
+                await updateBinding(editingShortcutId, originalBinding);
+                await invoke("resume_binding", { id: editingShortcutId }).catch(
+                  console.error,
+                );
+              } catch (resetError) {
+                console.error("Failed to reset binding:", resetError);
+                toast.error("Failed to reset shortcut to original value");
+              }
+            }
           }
 
           // Exit editing mode and reset states
@@ -132,10 +166,25 @@ export const HandyShortcut: React.FC<HandyShortcutProps> = ({
     };
 
     // Add click outside handler
-    const handleClickOutside = (e: MouseEvent) => {
+    const handleClickOutside = async (e: MouseEvent) => {
       const activeElement = shortcutRefs.current.get(editingShortcutId);
       if (activeElement && !activeElement.contains(e.target as Node)) {
-        // Cancel shortcut recording - the hook will handle rollback
+        // Cancel shortcut recording and restore original binding
+        if (editingShortcutId && originalBinding) {
+          try {
+            await updateBinding(editingShortcutId, originalBinding);
+            await invoke("resume_binding", { id: editingShortcutId }).catch(
+              console.error,
+            );
+          } catch (error) {
+            console.error("Failed to restore original binding:", error);
+            toast.error("Failed to restore original shortcut");
+          }
+        } else if (editingShortcutId) {
+          invoke("resume_binding", { id: editingShortcutId }).catch(
+            console.error,
+          );
+        }
         setEditingShortcutId(null);
         setKeyPressed([]);
         setRecordedKeys([]);
@@ -159,12 +208,16 @@ export const HandyShortcut: React.FC<HandyShortcutProps> = ({
     bindings,
     originalBinding,
     updateBinding,
+    osType,
   ]);
 
   // Start recording a new shortcut
-  const startRecording = (id: string) => {
+  const startRecording = async (id: string) => {
     if (editingShortcutId === id) return; // Already editing this shortcut
 
+    // Suspend current binding to avoid firing while recording
+    await invoke("suspend_binding", { id }).catch(console.error);
+
     // Store the original binding to restore if canceled
     setOriginalBinding(bindings[id]?.current_binding || "");
     setEditingShortcutId(id);
@@ -173,15 +226,11 @@ export const HandyShortcut: React.FC<HandyShortcutProps> = ({
   };
 
   // Format the current shortcut keys being recorded
-  const formatCurrentKeys = () => {
+  const formatCurrentKeys = (): string => {
     if (recordedKeys.length === 0) return "Press keys...";
 
-    if (!isMacOS) {
+    // Use the same formatting as the display to ensure consistency
-      return recordedKeys.join("+");
+    return formatKeyCombination(recordedKeys.join("+"), osType);
-    }
-
-    // Map each key to its macOS-friendly name for display
-    return recordedKeys.map(formatMacOSKeys).join(" + ");
   };
 
   // Store references to shortcut elements
@@ -248,7 +297,7 @@ export const HandyShortcut: React.FC<HandyShortcutProps> = ({
                 className="px-2 py-1 text-sm font-semibold bg-mid-gray/10 border border-mid-gray/80 hover:bg-logo-primary/10 rounded cursor-pointer hover:border-logo-primary"
                 onClick={() => startRecording(primaryId)}
               >
-                {formatKeyCombination(primaryBinding.current_binding)}
+                {formatKeyCombination(primaryBinding.current_binding, osType)}
               </div>
             )}
             <button

src/lib/utils/keyboard.ts

@@ -2,12 +2,17 @@
  * Keyboard utility functions for handling keyboard events
  */
 
+export type OSType = "macos" | "windows" | "linux" | "unknown";
+
 /**
  * Extract a consistent key name from a KeyboardEvent
- * This function replaces the keycode library and provides better support
+ * This function provides cross-platform keyboard event handling
- * for extended function keys (F14+) and cross-platform compatibility
+ * and returns key names appropriate for the target operating system
  */
-export const getKeyName = (e: KeyboardEvent): string => {
+export const getKeyName = (
+  e: KeyboardEvent,
+  osType: OSType = "unknown",
+): string => {
   // Handle special cases first
   if (e.code) {
     const code = e.code;
@@ -32,18 +37,35 @@ export const getKeyName = (e: KeyboardEvent): string => {
       return code.replace("Numpad", "numpad ").toLowerCase();
     }
 
-    // Handle modifier keys - normalize left/right variants
+    // Handle modifier keys - OS-specific naming
+    const getModifierName = (baseModifier: string): string => {
+      switch (baseModifier) {
+        case "shift":
+          return "shift";
+        case "ctrl":
+          return osType === "macos" ? "ctrl" : "ctrl";
+        case "alt":
+          return osType === "macos" ? "option" : "alt";
+        case "meta":
+          // Windows key on Windows/Linux, Command key on Mac
+          if (osType === "macos") return "command";
+          return "super";
+        default:
+          return baseModifier;
+      }
+    };
+
     const modifierMap: Record<string, string> = {
-      ShiftLeft: "shift",
+      ShiftLeft: getModifierName("shift"),
-      ShiftRight: "shift",
+      ShiftRight: getModifierName("shift"),
-      ControlLeft: "ctrl",
+      ControlLeft: getModifierName("ctrl"),
-      ControlRight: "ctrl",
+      ControlRight: getModifierName("ctrl"),
-      AltLeft: "alt",
+      AltLeft: getModifierName("alt"),
-      AltRight: "alt",
+      AltRight: getModifierName("alt"),
-      MetaLeft: "command",
+      MetaLeft: getModifierName("meta"),
-      MetaRight: "command",
+      MetaRight: getModifierName("meta"),
-      OSLeft: "command",
+      OSLeft: getModifierName("meta"),
-      OSRight: "command",
+      OSRight: getModifierName("meta"),
       CapsLock: "caps lock",
       Tab: "tab",
       Enter: "enter",
@@ -103,13 +125,15 @@ export const getKeyName = (e: KeyboardEvent): string => {
   if (e.key) {
     const key = e.key;
 
-    // Handle special key names
+    // Handle special key names with OS-specific formatting
     const keyMap: Record<string, string> = {
-      Control: "ctrl",
+      Control: osType === "macos" ? "ctrl" : "ctrl",
-      Alt: "alt",
+      Alt: osType === "macos" ? "option" : "alt",
       Shift: "shift",
-      Meta: "command",
+      Meta:
-      OS: "command",
+        osType === "macos" ? "command" : osType === "windows" ? "win" : "super",
+      OS:
+        osType === "macos" ? "command" : osType === "windows" ? "win" : "super",
       CapsLock: "caps lock",
       ArrowUp: "up",
       ArrowDown: "down",
@@ -129,3 +153,31 @@ export const getKeyName = (e: KeyboardEvent): string => {
   // Last resort fallback
   return `unknown-${e.keyCode || e.which || 0}`;
 };
+
+/**
+ * Get display-friendly key combination string for the current OS
+ * Returns basic plus-separated format with correct platform key names
+ */
+export const formatKeyCombination = (
+  combination: string,
+  osType: OSType,
+): string => {
+  // Simply return the combination as-is since getKeyName already provides
+  // the correct platform-specific key names
+  return combination;
+};
+
+/**
+ * Normalize modifier keys to handle left/right variants
+ */
+export const normalizeKey = (key: string): string => {
+  // Handle left/right variants of modifier keys
+  if (key.startsWith("left ") || key.startsWith("right ")) {
+    const parts = key.split(" ");
+    if (parts.length === 2) {
+      // Return just the modifier name without left/right prefix
+      return parts[1];
+    }
+  }
+  return key;
+};