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>
This commit is contained in:
Kyle 2026-03-11 03:46:40 +08:00
parent a737f06e85
commit 4e91944956
3 changed files with 26 additions and 38 deletions

View File

@ -20,6 +20,7 @@ import { Select } from '@/components/ui/select';
import { Sheet, SheetContent, SheetClose } from '@/components/ui/sheet'; import { Sheet, SheetContent, SheetClose } from '@/components/ui/sheet';
import CalendarSidebar from './CalendarSidebar'; import CalendarSidebar from './CalendarSidebar';
import EventDetailPanel from './EventDetailPanel'; import EventDetailPanel from './EventDetailPanel';
import MobileDetailOverlay from '@/components/shared/MobileDetailOverlay';
import type { CreateDefaults } from './EventDetailPanel'; import type { CreateDefaults } from './EventDetailPanel';
type CalendarView = 'dayGridMonth' | 'timeGridWeek' | 'timeGridDay'; type CalendarView = 'dayGridMonth' | 'timeGridWeek' | 'timeGridDay';
@ -639,26 +640,18 @@ export default function CalendarPage() {
{/* Mobile detail panel overlay */} {/* Mobile detail panel overlay */}
{panelOpen && !isDesktop && ( {panelOpen && !isDesktop && (
<div <MobileDetailOverlay open onClose={handlePanelClose} className="sm:max-w-[400px]">
className="fixed inset-0 z-50 bg-background/80 backdrop-blur-sm" <EventDetailPanel
onClick={handlePanelClose} event={panelMode === 'view' ? selectedEvent : null}
> isCreating={panelMode === 'create'}
<div createDefaults={createDefaults}
className="fixed inset-y-0 right-0 w-full sm:w-[400px] bg-card border-l border-border shadow-lg overflow-y-auto" onClose={handlePanelClose}
onClick={(e) => e.stopPropagation()} onSaved={handlePanelClose}
> locationName={selectedEvent?.location_id ? locationMap.get(selectedEvent.location_id) : undefined}
<EventDetailPanel myPermission={selectedEventPermission}
event={panelMode === 'view' ? selectedEvent : null} isSharedEvent={selectedEventIsShared}
isCreating={panelMode === 'create'} />
createDefaults={createDefaults} </MobileDetailOverlay>
onClose={handlePanelClose}
onSaved={handlePanelClose}
locationName={selectedEvent?.location_id ? locationMap.get(selectedEvent.location_id) : undefined}
myPermission={selectedEventPermission}
isSharedEvent={selectedEventIsShared}
/>
</div>
</div>
)} )}
</div> </div>
); );

View File

@ -145,24 +145,15 @@ export function EntityTable<T extends { id: number }>({
<div className="flex items-center gap-2 justify-end"> <div className="flex items-center gap-2 justify-end">
<ChevronsUpDown className="h-3.5 w-3.5 text-muted-foreground" /> <ChevronsUpDown className="h-3.5 w-3.5 text-muted-foreground" />
<select <select
value={`${sortKey}:${sortDir}`} value={sortKey}
onChange={(e) => { onChange={(e) => onSort(e.target.value)}
const [key, dir] = e.target.value.split(':'); aria-label="Sort by"
// 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);
}
}}
className="h-7 rounded-md border border-border bg-card px-2 text-xs text-foreground" className="h-7 rounded-md border border-border bg-card px-2 text-xs text-foreground"
> >
{sortableColumns.map((col) => ( {sortableColumns.map((col) => (
<React.Fragment key={col.key}> <option key={col.key} value={col.key}>
<option value={`${col.key}:asc`}>{col.label} </option> {col.label} {sortKey === col.key ? (sortDir === 'asc' ? '↑' : '↓') : ''}
<option value={`${col.key}:desc`}>{col.label} </option> </option>
</React.Fragment>
))} ))}
</select> </select>
</div> </div>

View File

@ -1,4 +1,4 @@
import { useEffect } from 'react'; import { useEffect, useRef } from 'react';
import { cn } from '@/lib/utils'; import { cn } from '@/lib/utils';
interface MobileDetailOverlayProps { interface MobileDetailOverlayProps {
@ -20,15 +20,19 @@ export default function MobileDetailOverlay({
children, children,
className, className,
}: MobileDetailOverlayProps) { }: MobileDetailOverlayProps) {
// Stable ref to avoid re-registering listener on every render
const onCloseRef = useRef(onClose);
onCloseRef.current = onClose;
// Escape key handler // Escape key handler
useEffect(() => { useEffect(() => {
if (!open) return; if (!open) return;
const handler = (e: KeyboardEvent) => { const handler = (e: KeyboardEvent) => {
if (e.key === 'Escape') onClose(); if (e.key === 'Escape') onCloseRef.current();
}; };
document.addEventListener('keydown', handler); document.addEventListener('keydown', handler);
return () => document.removeEventListener('keydown', handler); return () => document.removeEventListener('keydown', handler);
}, [open, onClose]); }, [open]);
// Body scroll lock // Body scroll lock
useEffect(() => { useEffect(() => {