Add keyboard shortcuts support for improved navigation

[dubadub]

Summary

This PR adds comprehensive keyboard shortcuts support to the Cook CLI web interface, enabling users to navigate and interact with the application more efficiently without relying on mouse clicks.

Key Changes

• New keyboard shortcuts module (static/js/keyboard-shortcuts.js):

  • Global navigation shortcuts (e.g., / for search, g h for home, g s for shopping list, g p for pantry, g x for preferences)
  • General shortcuts (? for help, t for theme toggle, Esc for closing modals)
  • Recipe page shortcuts (e to edit, a to add to shopping list, p to print, +/- and [/] to adjust recipe scale)
  • Shopping list shortcuts (c to clear all items)
  • Multi-key sequence support (e.g., "g h" for navigation)
  • Smart input detection to avoid interfering with text input fields, textareas, and code editors

• Updated base template (templates/base.html):

  • Added keyboard shortcuts help button in the navigation bar with a question mark icon
  • Integrated the keyboard shortcuts JavaScript module
  • Added fallback function to ensure showShortcutsHelp() is available globally

Implementation Details

• Input awareness: The module intelligently detects when users are typing in input fields, textareas, contenteditable elements, or CodeMirror editors and disables shortcuts accordingly
• Modal UI: Keyboard shortcuts help is displayed in a responsive modal with dark mode support, organized into categories (Navigation, General, Recipe Page, Shopping List)
• Sequence handling: Multi-key shortcuts like "g h" are handled with a 1.5-second timeout to allow for natural typing
• Accessibility: Escape key closes modals and blurs input fields; all shortcuts are documented in the help modal
• Non-intrusive: Shortcuts only trigger when appropriate and don't interfere with standard browser behavior or user input

Testing Recommendations

• Test all keyboard shortcuts on different pages (home, recipe, shopping list, pantry, preferences)
• Verify shortcuts don't interfere when typing in search, input fields, or text areas
• Test multi-key sequences (e.g., "g h", "g s") with various timing
• Verify modal opens/closes correctly with ? and Esc keys
• Test theme toggle and other global shortcuts
• Verify recipe scale adjustment works with +/- and [/] keys

https://claude.ai/code/session_016paFwPNACFdAEmYmg9txDF

[claude]

Pull Request Review: Keyboard Shortcuts Support

Thank you for this comprehensive PR adding keyboard shortcuts to CookCLI! This is a well-thought-out feature that will significantly improve user experience. Below is my detailed review:

---

✅ Strengths

1. Code Quality & Best Practices

• Excellent IIFE pattern: The self-executing function prevents global namespace pollution
• Good input detection logic: The isTyping() function properly handles input fields, textareas, contenteditable elements, and CodeMirror editors
• Multi-key sequence support: The "g h" style shortcuts with timeout are well-implemented and user-friendly
• Event delegation: Proper use of addEventListener and event handling
• Clean modal implementation: HTML string generation is readable and well-structured
• Consistent code style: Follows JavaScript best practices with clear variable naming

2. User Experience

• Smart shortcut choices: The shortcuts follow common patterns (e.g., / for search, ? for help, Esc to close)
• Non-intrusive: Properly disables when users are typing
• Discoverable: Help modal (?) makes shortcuts easy to learn
• Keyboard navigation: Excellent accessibility feature for power users

3. Integration

• Minimal template changes: Only adds necessary button and script tags
• Graceful degradation: Fallback function ensures the app won't break if script fails to load
• Dark mode support: Modal respects the existing theme system

---

🐛 Potential Issues & Bugs

1. Theme Toggle Function Availability (templates/base.html:580)

if (typeof toggleTheme === 'function') {
    toggleTheme();
}

Issue: The code checks if toggleTheme exists before calling it, but doesn't verify where it's defined in base.html. If this function is defined later in the template or in a different script, the t shortcut might fail silently on some pages.

Recommendation: Review base.html to ensure toggleTheme is always available, or add console logging for debugging.

2. Global Function Dependencies (keyboard-shortcuts.js:369)

if (typeof clearList === 'function') {
    if (confirm('Clear all items from shopping list?')) {
        clearList();
    }
}

Issue: Similar issue - depends on clearList being globally available. If it's not defined on the shopping list page, the shortcut silently fails.

Recommendation: Verify that all page-specific functions are properly exposed or add error handling/logging.

3. Modal Backdrop Click Handler (keyboard-shortcuts.js:166-170)

modal.addEventListener('click', function(e) {
    if (e.target === modal) {
        closeShortcutsHelp();
    }
});

Potential Issue: Event listener is added every time the modal is shown (if it doesn't exist). If the modal is shown/hidden multiple times, you could accumulate event listeners.

Recommendation: Either:

• Add the listener only once when creating the modal
• Remove the listener when closing
• Check if listener already exists before adding

4. Scale Adjustment Precision (keyboard-shortcuts.js:345-347)

// Round to avoid floating point issues
newValue = Math.round(newValue * 10) / 10;

Issue: This limits precision to 1 decimal place. If the scale input supports more precision, this could truncate values.

Recommendation: Check the actual precision requirements of the scale input or make this configurable.

5. Missing Error Handling for DOM Elements

Several functions assume DOM elements exist without checking:

• adjustScale() checks if scaleInput exists, but other shortcuts don't always verify element existence
• Could lead to silent failures

Recommendation: Add defensive checks or error logging throughout.

---

🔒 Security Considerations

1. XSS Risk in Modal HTML (keyboard-shortcuts.js:71-161)

modal.innerHTML = `...`;

Assessment: ✅ Low Risk - The HTML is hardcoded and doesn't include any user input, so there's no XSS vulnerability here. However, be cautious if this pattern is copied elsewhere with dynamic content.

2. Global Function Exposure

window.showShortcutsHelp = function() { ... }
window.closeShortcutsHelp = function() { ... }

Assessment: ✅ Acceptable - These functions need to be global for the onclick handlers. No security concerns.

---

⚡ Performance Considerations

1. Event Listener on Every Keydown (keyboard-shortcuts.js:388)

document.addEventListener('keydown', handleKeydown);

Assessment: ✅ Good - Single global listener is efficient. The isTyping() check returns early, so minimal overhead.

2. DOM Queries in Event Handlers

Multiple shortcuts use querySelector and querySelectorAll during keypress:

const searchInput = document.getElementById('search-input');
const editLink = document.querySelector('a[href^="/edit/"]');

Issue: ⚠️ Minor - These queries happen on every relevant keypress. For better performance, consider caching element references, especially for frequently accessed elements.

Recommendation:

// Cache on DOMContentLoaded
let cachedElements = {};
document.addEventListener('DOMContentLoaded', function() {
    cachedElements.searchInput = document.getElementById('search-input');
});

3. Modal Recreation

The modal checks if it exists before creating a new one:

const existingModal = document.getElementById('keyboard-shortcuts-modal');
if (existingModal) {
    existingModal.classList.remove('hidden');
    return;
}

Assessment: ✅ Good - Reuses existing modal, efficient approach.

---

🧪 Test Coverage

⚠️ CRITICAL: Missing Test Coverage

Based on the comprehensive Playwright test suite documented in CLAUDE.md, this PR must include E2E tests for the new keyboard shortcuts functionality.

Required Tests (should be in tests/e2e/keyboard-shortcuts.spec.ts):

import { test, expect } from '@playwright/test';
import { TestHelpers } from '../fixtures/test-helpers';

test.describe('Keyboard Shortcuts', () => {
  let helpers: TestHelpers;

  test.beforeEach(async ({ page }) => {
    helpers = new TestHelpers(page);
    await helpers.navigateTo('/');
  });

  test.describe('Global Shortcuts', () => {
    test('should focus search with / key', async ({ page }) => {
      await page.keyboard.press('/');
      const searchInput = page.locator('#search-input');
      await expect(searchInput).toBeFocused();
    });

    test('should show help modal with ? key', async ({ page }) => {
      await page.keyboard.press('Shift+/'); // ? key
      const modal = page.locator('#keyboard-shortcuts-modal');
      await expect(modal).toBeVisible();
    });

    test('should close modal with Escape key', async ({ page }) => {
      await page.keyboard.press('Shift+/');
      await page.keyboard.press('Escape');
      const modal = page.locator('#keyboard-shortcuts-modal');
      await expect(modal).toHaveClass(/hidden/);
    });

    test('should navigate to home with g h sequence', async ({ page }) => {
      await helpers.navigateTo('/shopping-list');
      await page.keyboard.press('g');
      await page.keyboard.press('h');
      expect(page.url()).toMatch(/\/$|recipes/);
    });

    test('should navigate to shopping list with g s sequence', async ({ page }) => {
      await page.keyboard.press('g');
      await page.keyboard.press('s');
      expect(page.url()).toContain('/shopping-list');
    });

    test('should toggle theme with t key', async ({ page }) => {
      const html = page.locator('html');
      const initialDark = await html.getAttribute('class');
      await page.keyboard.press('t');
      await page.waitForTimeout(100);
      const afterToggle = await html.getAttribute('class');
      expect(initialDark).not.toBe(afterToggle);
    });
  });

  test.describe('Recipe Page Shortcuts', () => {
    test.beforeEach(async ({ page }) => {
      // Navigate to a recipe page
      const recipeLink = page.locator('a[href^="/recipe/"]').first();
      await recipeLink.click();
      await page.waitForLoadState('networkidle');
    });

    test('should increase scale with + key', async ({ page }) => {
      const scaleInput = page.locator('#scale');
      const initialValue = await scaleInput.inputValue();
      await page.keyboard.press('+');
      const newValue = await scaleInput.inputValue();
      expect(parseFloat(newValue)).toBeGreaterThan(parseFloat(initialValue));
    });

    test('should decrease scale with - key', async ({ page }) => {
      const scaleInput = page.locator('#scale');
      await scaleInput.fill('2');
      await page.keyboard.press('-');
      const newValue = await scaleInput.inputValue();
      expect(parseFloat(newValue)).toBe(1.5);
    });

    test('should add to shopping list with a key', async ({ page }) => {
      await page.keyboard.press('a');
      // Verify shopping list action (depends on implementation)
      await expect(page.locator('text=Added to shopping list')).toBeVisible({ timeout: 2000 });
    });

    test('should print with p key', async ({ page }) => {
      const printPromise = page.waitForEvent('dialog');
      await page.keyboard.press('p');
      // Note: window.print() may not trigger in headless mode
    });
  });

  test.describe('Input Field Handling', () => {
    test('should not trigger shortcuts when typing in search', async ({ page }) => {
      const searchInput = page.locator('#search-input');
      await searchInput.click();
      await page.keyboard.type('pasta');
      // Verify we're still on the same page (g didn't navigate)
      expect(page.url()).toMatch(/\/$|recipes/);
    });

    test('should blur input field with Escape', async ({ page }) => {
      const searchInput = page.locator('#search-input');
      await searchInput.click();
      await page.keyboard.press('Escape');
      await expect(searchInput).not.toBeFocused();
    });
  });

  test.describe('Shopping List Shortcuts', () => {
    test.beforeEach(async ({ page }) => {
      await helpers.navigateTo('/shopping-list');
    });

    test('should clear list with c key', async ({ page }) => {
      page.on('dialog', dialog => dialog.accept());
      await page.keyboard.press('c');
      // Verify list cleared (depends on implementation)
    });
  });

  test.describe('Multi-key Sequence Timeout', () => {
    test('should timeout pending key sequence after 1.5s', async ({ page }) => {
      await page.keyboard.press('g');
      await page.waitForTimeout(2000);
      await page.keyboard.press('h');
      // Should not navigate since sequence timed out
      expect(page.url()).toMatch(/\/$|recipes/);
    });
  });
});

Additional Tests Needed:

1. Accessibility tests in accessibility.spec.ts - verify keyboard shortcuts don't break WCAG compliance
2. Mobile tests - ensure shortcuts don't interfere with mobile keyboard
3. Browser compatibility - test across Chrome, Firefox, Safari

---

📝 Additional Recommendations

1. Documentation

• Add a section to the README about keyboard shortcuts
• Consider adding aria-labels to the help button for screen readers
• Document the timeout value (1500ms) as a constant for easy configuration

2. User Feedback

• Consider adding visual feedback when multi-key sequences are in progress (e.g., show "g" in the corner)
• Add toast notifications for successful actions ("Added to shopping list", "Scale updated")

3. Code Organization

Consider extracting configuration to the top:

const CONFIG = {
    SEQUENCE_TIMEOUT: 1500,
    SCALE_MIN: 0.5,
    SCALE_MAX: 200,
    SCALE_STEP_SMALL: 0.5,
    SCALE_STEP_LARGE: 1.0,
    SCALE_PRECISION: 10 // For rounding
};

4. Browser Compatibility

Test with:

• Safari (Mac/iOS) - might have different keyboard event handling
• Firefox - ensure all shortcuts work
• Mobile browsers - virtual keyboards behave differently

5. Potential Conflicts

• Check if any shortcuts conflict with browser defaults (e.g., Cmd+P for print on Mac)
• Consider documenting known conflicts

---

📊 Summary

Category	Rating	Notes
Code Quality	⭐⭐⭐⭐½	Well-structured, minor improvements possible
Best Practices	⭐⭐⭐⭐⭐	Excellent use of IIFE, event delegation
Security	⭐⭐⭐⭐⭐	No security concerns identified
Performance	⭐⭐⭐⭐	Good, minor optimization opportunities
Test Coverage	⭐	CRITICAL: No tests included
Documentation	⭐⭐⭐	Good inline docs, needs user-facing docs
Accessibility	⭐⭐⭐⭐	Good keyboard support, minor improvements possible

---

✅ Action Items Before Merge

Must Have:

1. ✅ Add comprehensive Playwright tests (see test examples above)
2. ✅ Verify toggleTheme and clearList functions are available
3. ✅ Test on Firefox, Safari, and mobile browsers

Should Have:
4. ⚠️ Add defensive null checks for DOM element queries
5. ⚠️ Fix potential memory leak with modal event listeners
6. ⚠️ Add README documentation for keyboard shortcuts

Nice to Have:
7. 💡 Consider caching frequently accessed DOM elements
8. 💡 Add visual feedback for multi-key sequences
9. 💡 Extract magic numbers to configuration constants

---

🎯 Conclusion

This is a high-quality PR that adds significant value to CookCLI. The implementation is clean, user-friendly, and follows JavaScript best practices. The main blocker is the lack of test coverage, which is critical given the project's comprehensive testing infrastructure.

Recommendation: Request changes to add Playwright tests before merging. Once tests are added and the minor issues are addressed, this PR will be ready to merge.

Great work overall! 🎉

[claude]

Code Review - PR #252: Add keyboard shortcuts support

Summary

This PR adds comprehensive keyboard shortcuts to the web UI, which is a great UX enhancement. The implementation is generally well-structured with good input detection and modal UI. However, there are several areas that need attention before merging.

Critical Issues

1. Security: Inline Event Handlers

Location: templates/base.html:501 and base.html:580-593

The PR uses inline onclick handlers, which violates Content Security Policy (CSP) best practices.

Recommendation: Use event listeners instead of inline handlers.

2. Race Condition: Function Availability

Location: templates/base.html:586-592

The fallback check for showShortcutsHelp may not work reliably - the script runs immediately but keyboard-shortcuts.js is loaded via a non-blocking script tag.

Recommendation: Remove inline handlers or ensure the script loads synchronously.

3. Missing Dependency Functions

Location: static/js/keyboard-shortcuts.js:268, js:377

The code calls functions that may not exist: toggleTheme() and clearList()

Recommendation: Add defensive checks before calling these functions.

Code Quality Issues

4. Inconsistent Scale Adjustment

Location: static/js/keyboard-shortcuts.js:345-360

The scale adjustment dispatches a change event, but it's unclear if the scale input has a listener for this event.

5. Magic Numbers and Hardcoded Values

Location: static/js/keyboard-shortcuts.js:241, js:348-349

Values like 1500ms timeout, 0.5 min scale, 200 max scale should be extracted as named constants.

6. Global Namespace Pollution

The IIFE pattern is good, but showShortcutsHelp and closeShortcutsHelp are added to the global window object unnecessarily.

Testing Recommendations

7. No E2E Tests for Keyboard Shortcuts

The PR adds significant UI functionality but doesn't include Playwright tests. Based on the excellent test infrastructure in tests/e2e/, this should have tests for help modal, navigation shortcuts, recipe shortcuts, and input field detection.

Positive Aspects

✅ Excellent input detection - The isTyping() function properly handles inputs, textareas, contenteditable, and CodeMirror
✅ Good UX - Multi-key sequences follow familiar vim-style conventions
✅ Comprehensive shortcuts - Covers navigation, recipe management, and shopping list
✅ Accessibility - Escape key handling and keyboard navigation support
✅ Clean modal UI - Well-styled help modal with dark mode support
✅ Good clippy fixes - The Rust changes properly use if let instead of unwrap

Verdict

Changes Requested - The feature is valuable but needs the critical security and reliability issues addressed before merging.

Required Changes:

1. Remove inline onclick handlers and use event listeners
2. Add defensive checks for toggleTheme() and clearList() functions
3. Verify scale adjustment compatibility with existing recipe page code
4. Add E2E tests for keyboard shortcuts functionality

Recommended Changes:

5. Extract magic numbers to named constants
6. Add better error handling for missing elements
7. Consider performance optimizations for modal creation and path checking

Once these issues are addressed, this will be a great addition to CookCLI!