Fix QA review issues: error handlers, validation, accessibility, cleanup

- C1: Add onError handlers to dismiss/snooze mutations in useAlerts
- C2: Clear snoozed_until when dismissing via update endpoint
- W1: Handle future dates in getRelativeTime
- W2+S3: Add Escape key, aria-expanded, role=menu to SnoozeDropdown
- W4: Remove redundant field_validator (Literal suffices)
- W7: Validate recurrence_rule with Literal['daily','weekly','monthly']
- S2: Clean up delete confirmation setTimeout on unmount
- S6: Cap AlertBanner height with scroll for many alerts

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Kyle 2026-02-24 03:52:28 +08:00
parent 758e794880
commit 17f331477f
9 changed files with 68 additions and 21 deletions

View File

@ -244,9 +244,27 @@ Several components feel like unstyled defaults:
- [x] Single-line compact row design (ReminderItem) matching TodoItem pattern
- [x] Grouped sections (Overdue → Today → Upcoming → No Date → Dismissed)
- [x] Recurrence badge inline, date coloring (red overdue, yellow today, muted future)
- [x] Dismiss button, edit button, 2-second confirm delete pattern
- [x] Dismiss button, edit button, 4-second confirm delete with "Sure?" label
- [x] Optimistic delete with rollback
- [x] Empty state with "Add Reminder" action button
- [x] ReminderForm uses single datetime-local input (matches EventForm) with recurrence alongside
#### Reminder Alerts — COMPLETED
- [x] Backend: `snoozed_until` column + Alembic migration 017 with composite index
- [x] Backend: `GET /api/reminders/due` endpoint (overdue, non-dismissed, non-recurring, snooze-aware)
- [x] Backend: `PATCH /api/reminders/{id}/snooze` with `Literal[5, 10, 15]` validation + state guards
- [x] Backend: Snooze/due endpoints accept `client_now` from frontend (fixes Docker UTC vs local time)
- [x] Backend: Dismiss clears `snoozed_until`; updating `remind_at` reactivates dismissed reminders
- [x] Frontend: `AlertsProvider` context in AppLayout — single polling instance, no duplicate toasts
- [x] Frontend: `useAlerts` hook polls `/reminders/due` every 30s with client_now
- [x] Frontend: Sonner custom toasts on non-dashboard pages (max 3 + overflow summary, `duration: Infinity`)
- [x] Frontend: `AlertBanner` on dashboard below stats row (orange left accent, compact rows)
- [x] Frontend: `SnoozeDropdown` component — clock icon + "Snooze" label, opens dropdown with 5/10/15 min options
- [x] Frontend: Toast/banner dismiss button with X icon + "Dismiss" label
- [x] Frontend: Route-aware display — toasts dismissed on dashboard entry, fired on exit
- [x] Frontend: Dashboard Active Reminders card filters out items already in AlertBanner
- [x] Frontend: Shared `getRelativeTime` + `toLocalDatetime` utilities in `lib/date-utils.ts`
- [x] Accessibility: aria-labels on all snooze/dismiss buttons
### Stage 5: Entity Pages (People, Locations)
- Avatar placeholders for People cards
@ -275,4 +293,4 @@ Several components feel like unstyled defaults:
## Refresh Scope
**Current status:** Stages 1-4 complete. Next up: Stage 5 Entity Pages (People, Locations).
**Current status:** Stages 1-4 complete, plus Reminder Alerts feature. Next up: Stage 5 Entity Pages (People, Locations).

View File

@ -144,6 +144,10 @@ async def update_reminder(
reminder.snoozed_until = None
reminder.is_dismissed = False
# Clear snoozed_until when dismissing via update (match dedicated endpoint)
if update_data.get('is_dismissed') is True:
reminder.snoozed_until = None
for key, value in update_data.items():
setattr(reminder, key, value)

View File

@ -1,4 +1,4 @@
from pydantic import BaseModel, ConfigDict, field_validator
from pydantic import BaseModel, ConfigDict
from datetime import datetime
from typing import Literal, Optional
@ -8,7 +8,7 @@ class ReminderCreate(BaseModel):
description: Optional[str] = None
remind_at: Optional[datetime] = None
is_active: bool = True
recurrence_rule: Optional[str] = None
recurrence_rule: Optional[Literal['daily', 'weekly', 'monthly']] = None
class ReminderUpdate(BaseModel):
@ -17,20 +17,13 @@ class ReminderUpdate(BaseModel):
remind_at: Optional[datetime] = None
is_active: Optional[bool] = None
is_dismissed: Optional[bool] = None
recurrence_rule: Optional[str] = None
recurrence_rule: Optional[Literal['daily', 'weekly', 'monthly']] = None
class ReminderSnooze(BaseModel):
minutes: Literal[5, 10, 15]
client_now: Optional[datetime] = None
@field_validator('minutes', mode='before')
@classmethod
def validate_minutes(cls, v: int) -> int:
if v not in (5, 10, 15):
raise ValueError('Snooze duration must be 5, 10, or 15 minutes')
return v
class ReminderResponse(BaseModel):
id: int

View File

@ -23,7 +23,7 @@ export default function AlertBanner({ alerts, onDismiss, onSnooze }: AlertBanner
{alerts.length}
</span>
</div>
<div className="divide-y divide-border">
<div className="divide-y divide-border max-h-48 overflow-y-auto">
{alerts.map((alert) => (
<div
key={alert.id}

View File

@ -1,4 +1,4 @@
import { useState } from 'react';
import { useState, useRef, useEffect } from 'react';
import { useMutation, useQueryClient } from '@tanstack/react-query';
import { toast } from 'sonner';
import { Bell, BellOff, Trash2, Pencil } from 'lucide-react';
@ -24,6 +24,8 @@ const QUERY_KEYS = [['reminders'], ['dashboard'], ['upcoming']] as const;
export default function ReminderItem({ reminder, onEdit }: ReminderItemProps) {
const queryClient = useQueryClient();
const [confirmingDelete, setConfirmingDelete] = useState(false);
const deleteTimerRef = useRef<ReturnType<typeof setTimeout>>();
useEffect(() => () => clearTimeout(deleteTimerRef.current), []);
const remindDate = reminder.remind_at ? parseISO(reminder.remind_at) : null;
const isOverdue = !reminder.is_dismissed && remindDate && isPast(remindDate) && !isToday(remindDate);
@ -70,9 +72,10 @@ export default function ReminderItem({ reminder, onEdit }: ReminderItemProps) {
const handleDelete = () => {
if (!confirmingDelete) {
setConfirmingDelete(true);
setTimeout(() => setConfirmingDelete(false), 4000);
deleteTimerRef.current = setTimeout(() => setConfirmingDelete(false), 4000);
return;
}
clearTimeout(deleteTimerRef.current);
deleteMutation.mutate();
setConfirmingDelete(false);
};

View File

@ -19,13 +19,20 @@ export default function SnoozeDropdown({ onSnooze, label, direction = 'up' }: Sn
useEffect(() => {
if (!open) return;
function handleKey(e: KeyboardEvent) {
if (e.key === 'Escape') setOpen(false);
}
function handleClick(e: MouseEvent) {
if (ref.current && !ref.current.contains(e.target as Node)) {
setOpen(false);
}
}
document.addEventListener('keydown', handleKey);
document.addEventListener('mousedown', handleClick);
return () => document.removeEventListener('mousedown', handleClick);
return () => {
document.removeEventListener('keydown', handleKey);
document.removeEventListener('mousedown', handleClick);
};
}, [open]);
return (
@ -33,18 +40,21 @@ export default function SnoozeDropdown({ onSnooze, label, direction = 'up' }: Sn
<button
onClick={() => setOpen(!open)}
aria-label={`Snooze "${label}"`}
aria-expanded={open}
aria-haspopup="menu"
className="flex items-center gap-1 px-1.5 py-1 rounded hover:bg-accent/10 hover:text-accent text-muted-foreground transition-colors"
>
<Clock className="h-3.5 w-3.5" />
<span className="text-[11px] font-medium">Snooze</span>
</button>
{open && (
<div className={`absolute right-0 w-32 rounded-md border bg-popover shadow-lg z-50 py-1 animate-fade-in ${
<div role="menu" className={`absolute right-0 w-32 rounded-md border bg-popover shadow-lg z-50 py-1 animate-fade-in ${
direction === 'up' ? 'bottom-full mb-1' : 'top-full mt-1'
}`}>
{OPTIONS.map((opt) => (
<button
key={opt.value}
role="menuitem"
onClick={() => { onSnooze(opt.value); setOpen(false); }}
className="flex items-center w-full px-3 py-1.5 text-xs hover:bg-card-elevated transition-colors text-muted-foreground hover:text-foreground"
>

View File

@ -1,4 +1,4 @@
import { useState } from 'react';
import { useState, useRef, useEffect } from 'react';
import { useMutation, useQueryClient } from '@tanstack/react-query';
import { toast } from 'sonner';
import { Trash2, Pencil, Calendar, Clock, AlertCircle, RefreshCw } from 'lucide-react';
@ -32,6 +32,8 @@ const QUERY_KEYS = [['todos'], ['dashboard'], ['upcoming']] as const;
export default function TodoItem({ todo, onEdit }: TodoItemProps) {
const queryClient = useQueryClient();
const [confirmingDelete, setConfirmingDelete] = useState(false);
const deleteTimerRef = useRef<ReturnType<typeof setTimeout>>();
useEffect(() => () => clearTimeout(deleteTimerRef.current), []);
const toggleMutation = useMutation({
mutationFn: async () => {
@ -76,10 +78,10 @@ export default function TodoItem({ todo, onEdit }: TodoItemProps) {
const handleDelete = () => {
if (!confirmingDelete) {
setConfirmingDelete(true);
// Auto-reset after 2 seconds if not confirmed
setTimeout(() => setConfirmingDelete(false), 4000);
deleteTimerRef.current = setTimeout(() => setConfirmingDelete(false), 4000);
return;
}
clearTimeout(deleteTimerRef.current);
deleteMutation.mutate();
setConfirmingDelete(false);
};

View File

@ -64,6 +64,10 @@ export function AlertsProvider({ children }: { children: ReactNode }) {
queryClient.invalidateQueries({ queryKey: ['reminders'] });
queryClient.invalidateQueries({ queryKey: ['dashboard'] });
},
onError: () => {
queryClient.invalidateQueries({ queryKey: ['reminders', 'due'] });
toast.error('Failed to dismiss reminder');
},
});
const snoozeMutation = useMutation({
@ -73,6 +77,10 @@ export function AlertsProvider({ children }: { children: ReactNode }) {
queryClient.invalidateQueries({ queryKey: ['reminders'] });
queryClient.invalidateQueries({ queryKey: ['dashboard'] });
},
onError: () => {
queryClient.invalidateQueries({ queryKey: ['reminders', 'due'] });
toast.error('Failed to snooze reminder');
},
});
const handleDismiss = useCallback((id: number) => {

View File

@ -8,8 +8,17 @@ export function getRelativeTime(dateStr: string): string {
const date = new Date(dateStr);
const now = new Date();
const diffMs = now.getTime() - date.getTime();
const diffMins = Math.floor(diffMs / 60000);
if (diffMs < 0) {
const futureMins = Math.floor(-diffMs / 60000);
if (futureMins < 1) return 'Just now';
if (futureMins < 60) return `in ${futureMins}m`;
const futureHours = Math.floor(futureMins / 60);
if (futureHours < 24) return `in ${futureHours}h`;
return `in ${Math.floor(futureHours / 24)}d`;
}
const diffMins = Math.floor(diffMs / 60000);
if (diffMins < 1) return 'Just now';
if (diffMins < 60) return `${diffMins}m ago`;
const diffHours = Math.floor(diffMins / 60);