From 4e9194495662d4dec611c64ccf1694e17a9e1305 Mon Sep 17 00:00:00 2001
From: Kyle Pope <kyle.a.pope@outlook.com>
Date: Wed, 11 Mar 2026 03:46:40 +0800
Subject: [PATCH] Fix code review findings: sort dropdown, overlay ref,
 CalendarPage

- C-01: Simplify EntityTable sort dropdown to toggle-based (select
  column, re-select to flip direction), add aria-label
- W-01: Convert CalendarPage mobile overlay to MobileDetailOverlay
- W-02: Use ref for onClose in MobileDetailOverlay to prevent
  listener churn from inline arrow functions

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---
 .../src/components/calendar/CalendarPage.tsx  | 33 ++++++++-----------
 .../src/components/shared/EntityTable.tsx     | 21 ++++--------
 .../components/shared/MobileDetailOverlay.tsx | 10 ++++--
 3 files changed, 26 insertions(+), 38 deletions(-)

diff --git a/frontend/src/components/calendar/CalendarPage.tsx b/frontend/src/components/calendar/CalendarPage.tsx
index 794c8d8..e9d490c 100644
--- a/frontend/src/components/calendar/CalendarPage.tsx
+++ b/frontend/src/components/calendar/CalendarPage.tsx
@@ -20,6 +20,7 @@ import { Select } from '@/components/ui/select';
 import { Sheet, SheetContent, SheetClose } from '@/components/ui/sheet';
 import CalendarSidebar from './CalendarSidebar';
 import EventDetailPanel from './EventDetailPanel';
+import MobileDetailOverlay from '@/components/shared/MobileDetailOverlay';
 import type { CreateDefaults } from './EventDetailPanel';
 
 type CalendarView = 'dayGridMonth' | 'timeGridWeek' | 'timeGridDay';
@@ -639,26 +640,18 @@ export default function CalendarPage() {
 
       {/* Mobile detail panel overlay */}
       {panelOpen && !isDesktop && (
-        <div
-          className="fixed inset-0 z-50 bg-background/80 backdrop-blur-sm"
-          onClick={handlePanelClose}
-        >
-          <div
-            className="fixed inset-y-0 right-0 w-full sm:w-[400px] bg-card border-l border-border shadow-lg overflow-y-auto"
-            onClick={(e) => e.stopPropagation()}
-          >
-            <EventDetailPanel
-              event={panelMode === 'view' ? selectedEvent : null}
-              isCreating={panelMode === 'create'}
-              createDefaults={createDefaults}
-              onClose={handlePanelClose}
-              onSaved={handlePanelClose}
-              locationName={selectedEvent?.location_id ? locationMap.get(selectedEvent.location_id) : undefined}
-              myPermission={selectedEventPermission}
-              isSharedEvent={selectedEventIsShared}
-            />
-          </div>
-        </div>
+        <MobileDetailOverlay open onClose={handlePanelClose} className="sm:max-w-[400px]">
+          <EventDetailPanel
+            event={panelMode === 'view' ? selectedEvent : null}
+            isCreating={panelMode === 'create'}
+            createDefaults={createDefaults}
+            onClose={handlePanelClose}
+            onSaved={handlePanelClose}
+            locationName={selectedEvent?.location_id ? locationMap.get(selectedEvent.location_id) : undefined}
+            myPermission={selectedEventPermission}
+            isSharedEvent={selectedEventIsShared}
+          />
+        </MobileDetailOverlay>
       )}
     </div>
   );
diff --git a/frontend/src/components/shared/EntityTable.tsx b/frontend/src/components/shared/EntityTable.tsx
index 6204150..22f5823 100644
--- a/frontend/src/components/shared/EntityTable.tsx
+++ b/frontend/src/components/shared/EntityTable.tsx
@@ -145,24 +145,15 @@ export function EntityTable<T extends { id: number }>({
           <div className="flex items-center gap-2 justify-end">
             <ChevronsUpDown className="h-3.5 w-3.5 text-muted-foreground" />
             <select
-              value={`${sortKey}:${sortDir}`}
-              onChange={(e) => {
-                const [key, dir] = e.target.value.split(':');
-                // Toggle sort via onSort — if key matches current, it flips direction
-                // If different key, set new key. We call onSort which handles the logic.
-                if (key !== sortKey) {
-                  onSort(key);
-                } else if (dir !== sortDir) {
-                  onSort(key);
-                }
-              }}
+              value={sortKey}
+              onChange={(e) => onSort(e.target.value)}
+              aria-label="Sort by"
               className="h-7 rounded-md border border-border bg-card px-2 text-xs text-foreground"
             >
               {sortableColumns.map((col) => (
-                <React.Fragment key={col.key}>
-                  <option value={`${col.key}:asc`}>{col.label} ↑</option>
-                  <option value={`${col.key}:desc`}>{col.label} ↓</option>
-                </React.Fragment>
+                <option key={col.key} value={col.key}>
+                  {col.label} {sortKey === col.key ? (sortDir === 'asc' ? '↑' : '↓') : ''}
+                </option>
               ))}
             </select>
           </div>
diff --git a/frontend/src/components/shared/MobileDetailOverlay.tsx b/frontend/src/components/shared/MobileDetailOverlay.tsx
index 81cc7a6..93019ff 100644
--- a/frontend/src/components/shared/MobileDetailOverlay.tsx
+++ b/frontend/src/components/shared/MobileDetailOverlay.tsx
@@ -1,4 +1,4 @@
-import { useEffect } from 'react';
+import { useEffect, useRef } from 'react';
 import { cn } from '@/lib/utils';
 
 interface MobileDetailOverlayProps {
@@ -20,15 +20,19 @@ export default function MobileDetailOverlay({
   children,
   className,
 }: MobileDetailOverlayProps) {
+  // Stable ref to avoid re-registering listener on every render
+  const onCloseRef = useRef(onClose);
+  onCloseRef.current = onClose;
+
   // Escape key handler
   useEffect(() => {
     if (!open) return;
     const handler = (e: KeyboardEvent) => {
-      if (e.key === 'Escape') onClose();
+      if (e.key === 'Escape') onCloseRef.current();
     };
     document.addEventListener('keydown', handler);
     return () => document.removeEventListener('keydown', handler);
-  }, [open, onClose]);
+  }, [open]);
 
   // Body scroll lock
   useEffect(() => {