Improve accessibility and apply React best practices

[alecf]

Summary

• Accessibility: Add focus-visible ring styles to all interactive elements (Card, TabBar, SidebarLink, SidebarSearch, ContentSidebarTrigger), aria-label on icon-only buttons, aria-hidden on decorative icons, tabular-nums for numeric values in StatCard
• Animation: Extend prefers-reduced-motion to cover all animations (dropdown, tooltip, modal) — previously only covered skeleton/page-enter
• React performance: Memoize SidebarContext value to prevent unnecessary consumer re-renders, extract shared useEscapeKey hook to deduplicate escape key listeners, use ternary conditionals for clearer rendering, destructure props in StatGrid map callback

Test plan

• Verify focus rings are visible when tabbing through interactive Card, TabBar, SidebarLink, and SidebarSearch
• Verify prefers-reduced-motion: reduce disables all animations (dropdown, tooltip, modal)
• Verify sidebar collapse/expand behavior unchanged after context memoization
• Verify Escape key still closes mobile sidebar and content sidebar drawer
• Run npm run build and npm run lint — both pass

🤖 Generated with Claude Code

[github-actions]

🧹 Preview deployment has been cleaned up.

[claude]

Pull Request Review: Accessibility and React Best Practices

I've reviewed PR #31 and overall this is excellent work. The changes demonstrate strong attention to accessibility, performance optimization, and code quality. Here's my detailed feedback:

---

✅ Strengths

Accessibility Improvements

• Focus-visible rings: Properly added to all interactive elements (Card, TabBar, SidebarLink, SidebarSearch, ContentSidebarTrigger) following the design system's orange-500 standard
• ARIA labels: Correctly added aria-label to icon-only buttons and aria-hidden="true" to decorative icons
• Tabular numbers: Smart addition of tabular-nums to StatCard for proper numeric alignment
• Reduced motion: Comprehensive coverage now includes all animations (dropdown, tooltip, modal) - great attention to detail

React Performance Optimizations

• Context memoization: SidebarContext value is now properly memoized to prevent unnecessary re-renders in consumer components
• Hook extraction: The new useEscapeKey hook eliminates code duplication across Sidebar and ContentSidebarDrawer
• Clearer rendering: Ternary conditionals (condition ? <div /> : null) are more explicit than && for JSX rendering
• Destructuring in loops: StatGrid now destructures item properties upfront, improving readability and potentially micro-optimizations

Code Quality

• Well-documented hook: useEscapeKey has clear JSDoc with usage example
• Consistent patterns: Changes follow established codebase conventions (cn utility, Tailwind classes)
• Proper exports: Hook is correctly exported from both hooks/index.ts and main index.ts

---

🔍 Issues & Concerns

1. CRITICAL: useEscapeKey Hook Has Stale Closure Bug (src/hooks/useEscapeKey.ts:27)

The handler function is included in the dependency array, which will cause the effect to re-run every time a new function reference is passed. This is problematic because:

// In Sidebar.tsx:38-39
const closeSidebar = useCallback(() => setOpen(false), [setOpen]);
useEscapeKey(closeSidebar, isMobile && isOpen);

While closeSidebar is memoized with useCallback, the setOpen function from context could change on every render if the context value isn't stable (though you've fixed this with useMemo).

Recommendation: Use useRef to store the handler and avoid re-subscribing to the event listener unnecessarily:

export function useEscapeKey(handler: () => void, enabled = true) {
  const handlerRef = useRef(handler);
  
  useEffect(() => {
    handlerRef.current = handler;
  }, [handler]);

  useEffect(() => {
    if (!enabled) return;

    const handleEscape = (e: KeyboardEvent) => {
      if (e.key === "Escape") {
        handlerRef.current();
      }
    };

    document.addEventListener("keydown", handleEscape);
    return () => document.removeEventListener("keydown", handleEscape);
  }, [enabled]); // Only re-run when enabled changes
}

This pattern ensures the listener isn't constantly added/removed while keeping the handler always current.

2. Potential Performance Issue: Multiple Event Listeners (src/hooks/useEscapeKey.ts)

If multiple components use useEscapeKey simultaneously, you'll have multiple document-level event listeners for the same key. This isn't a bug per se, but could be optimized with a single listener and subscription pattern. However, given the limited usage (2 places), this is acceptable for now.

3. Missing Unit Tests

No tests were added for the new useEscapeKey hook. While the repository has minimal test coverage overall (only 2 test files found), new utilities should ideally have tests.

Recommendation: Add a test file src/hooks/useEscapeKey.test.ts:

import { renderHook } from '@testing-library/react';
import { useEscapeKey } from './useEscapeKey';

describe('useEscapeKey', () => {
  it('calls handler when Escape is pressed and enabled', () => {
    const handler = jest.fn();
    renderHook(() => useEscapeKey(handler, true));
    
    const event = new KeyboardEvent('keydown', { key: 'Escape' });
    document.dispatchEvent(event);
    
    expect(handler).toHaveBeenCalledTimes(1);
  });

  it('does not call handler when disabled', () => {
    const handler = jest.fn();
    renderHook(() => useEscapeKey(handler, false));
    
    const event = new KeyboardEvent('keydown', { key: 'Escape' });
    document.dispatchEvent(event);
    
    expect(handler).not.toHaveBeenCalled();
  });
});

---

💡 Suggestions for Improvement

4. StatGrid Destructuring Creates Unnecessary Constant (src/components/StatGrid/StatGrid.tsx:208)

const statusClass = statusClasses[status || 'default']

This creates a constant that's only used twice in close proximity. The original inline usage was fine and possibly more readable. This is a very minor point and mostly stylistic.

5. Consider Keyboard Event Options

The useEscapeKey hook doesn't prevent default behavior or stop propagation. For most cases this is fine, but you might want to consider:

const handleEscape = (e: KeyboardEvent) => {
  if (e.key === "Escape") {
    e.preventDefault(); // Prevent browser default (like exiting fullscreen)
    handler();
  }
};

However, this could interfere with other escape handlers (like closing a modal that contains a sidebar), so the current behavior is probably correct for this use case.

---

🔒 Security Review

No security concerns identified. Changes are UI-focused with no:

• User input handling modifications
• Authentication/authorization changes
• External API calls
• Potential XSS vectors
• SQL injection risks

---

🎯 Performance Considerations

Positive impacts:

• Context memoization reduces unnecessary re-renders across the component tree
• Destructuring in StatGrid is neutral to slightly positive
• useEscapeKey consolidation eliminates duplicate effect subscriptions

No negative impacts detected.

---

📋 Test Coverage Assessment

Current state: Repository has minimal test coverage (2 test files found)

This PR: No tests added for new hook or modified components

Recommendation: While not blocking for merge, consider:

1. Adding tests for useEscapeKey hook (shown above)
2. Adding accessibility tests for focus-visible rings using @testing-library/user-event's tab() method
3. Testing reduced-motion media query behavior

---

🎨 Design System Compliance

All changes align with DESIGN_GUIDE.md and CLAUDE.md:

• ✅ Orange-500 used for focus rings (primary action color)
• ✅ Consistent spacing and typography maintained
• ✅ Dark mode classes properly applied
• ✅ No application-specific logic introduced
• ✅ Components remain generic and reusable
• ✅ Following existing patterns (cn utility, compound components)

---

📝 Final Recommendation

Status: Approve with Minor Fixes Recommended

This PR significantly improves the library's accessibility and code quality. The only critical issue is the potential stale closure in useEscapeKey (Issue #1), which should be fixed before merge.

Action items before merge:

1. 🔴 Fix the useEscapeKey hook to use useRef pattern (critical)
2. 🟡 Add unit tests for the new hook (recommended)

Optional improvements for follow-up:

• Consider adding accessibility tests
• Consider documenting the focus-visible pattern in DESIGN_GUIDE.md for future reference

Excellent work on this PR! The accessibility improvements in particular are very well thought out.