From 8cbc95939a4836b69692e2f4b803586d715e81b5 Mon Sep 17 00:00:00 2001 From: Kyle Pope Date: Tue, 24 Feb 2026 21:19:22 +0800 Subject: [PATCH] Fix issues from QA review: schema safety, search scope, clipboard handling, UX polish - Remove `name` from PersonUpdate schema (always computed, prevents bypass) - Auto-split legacy `name` into first/last on create when only name provided - Expand backend search to cover first_name, last_name, nickname, email, company - Add server_default=text('false') to is_favourite and is_frequent model columns - Add .catch() to clipboard API call in CopyableField - Extract duplicate renderHeader into shared function in PeoplePage - Add Escape key handler to close mobile detail panel overlays - Extract calculate() out of useTableVisibility effects to single function - Guard getInitials against empty string input Co-Authored-By: Claude Opus 4.6 --- backend/app/models/location.py | 4 +- backend/app/models/person.py | 4 +- backend/app/routers/people.py | 22 ++++++- backend/app/schemas/person.py | 6 +- .../components/locations/LocationsPage.tsx | 12 +++- frontend/src/components/people/PeoplePage.tsx | 61 +++++++++---------- .../src/components/shared/CopyableField.tsx | 2 + frontend/src/components/shared/utils.ts | 1 + frontend/src/hooks/useTableVisibility.ts | 32 ++++------ 9 files changed, 82 insertions(+), 62 deletions(-) diff --git a/backend/app/models/location.py b/backend/app/models/location.py index 2830fc3..7dbea4a 100644 --- a/backend/app/models/location.py +++ b/backend/app/models/location.py @@ -1,4 +1,4 @@ -from sqlalchemy import String, Text, Boolean, func +from sqlalchemy import String, Text, Boolean, func, text from sqlalchemy.orm import Mapped, mapped_column, relationship from datetime import datetime from typing import Optional, List @@ -12,7 +12,7 @@ class Location(Base): name: Mapped[str] = mapped_column(String(255), nullable=False) address: Mapped[str] = mapped_column(Text, nullable=False) category: Mapped[str] = mapped_column(String(100), default="other") - is_frequent: Mapped[bool] = mapped_column(Boolean, nullable=False, default=False) + is_frequent: Mapped[bool] = mapped_column(Boolean, nullable=False, default=False, server_default=text('false')) contact_number: Mapped[Optional[str]] = mapped_column(String(50), nullable=True) email: Mapped[Optional[str]] = mapped_column(String(255), nullable=True) notes: Mapped[Optional[str]] = mapped_column(Text, nullable=True) diff --git a/backend/app/models/person.py b/backend/app/models/person.py index afae961..d489879 100644 --- a/backend/app/models/person.py +++ b/backend/app/models/person.py @@ -1,4 +1,4 @@ -from sqlalchemy import String, Text, Date, Boolean, func +from sqlalchemy import String, Text, Date, Boolean, func, text from sqlalchemy.orm import Mapped, mapped_column, relationship as sa_relationship from datetime import datetime, date from typing import Optional, List @@ -20,7 +20,7 @@ class Person(Base): first_name: Mapped[Optional[str]] = mapped_column(String(100), nullable=True) last_name: Mapped[Optional[str]] = mapped_column(String(100), nullable=True) nickname: Mapped[Optional[str]] = mapped_column(String(100), nullable=True) - is_favourite: Mapped[bool] = mapped_column(Boolean, nullable=False, default=False) + is_favourite: Mapped[bool] = mapped_column(Boolean, nullable=False, default=False, server_default=text('false')) company: Mapped[Optional[str]] = mapped_column(String(255), nullable=True) job_title: Mapped[Optional[str]] = mapped_column(String(255), nullable=True) mobile: Mapped[Optional[str]] = mapped_column(String(50), nullable=True) diff --git a/backend/app/routers/people.py b/backend/app/routers/people.py index a87bec7..962f767 100644 --- a/backend/app/routers/people.py +++ b/backend/app/routers/people.py @@ -1,6 +1,6 @@ from fastapi import APIRouter, Depends, HTTPException, Query from sqlalchemy.ext.asyncio import AsyncSession -from sqlalchemy import select +from sqlalchemy import select, or_ from datetime import datetime, timezone from typing import Optional, List @@ -39,7 +39,17 @@ async def get_people( query = select(Person) if search: - query = query.where(Person.name.ilike(f"%{search}%")) + term = f"%{search}%" + query = query.where( + or_( + Person.name.ilike(term), + Person.first_name.ilike(term), + Person.last_name.ilike(term), + Person.nickname.ilike(term), + Person.email.ilike(term), + Person.company.ilike(term), + ) + ) if category: query = query.where(Person.category == category) @@ -58,7 +68,13 @@ async def create_person( current_user: Settings = Depends(get_current_session) ): """Create a new person with denormalised display name.""" - new_person = Person(**person.model_dump()) + data = person.model_dump() + # Auto-split legacy name into first/last if only name is provided + if data.get('name') and not data.get('first_name') and not data.get('last_name') and not data.get('nickname'): + parts = data['name'].split(' ', 1) + data['first_name'] = parts[0] + data['last_name'] = parts[1] if len(parts) > 1 else None + new_person = Person(**data) new_person.name = _compute_display_name( new_person.first_name, new_person.last_name, diff --git a/backend/app/schemas/person.py b/backend/app/schemas/person.py index fb4850a..9e9ac8d 100644 --- a/backend/app/schemas/person.py +++ b/backend/app/schemas/person.py @@ -4,7 +4,7 @@ from typing import Optional class PersonCreate(BaseModel): - name: Optional[str] = None + name: Optional[str] = None # legacy fallback — auto-split into first/last if provided alone first_name: Optional[str] = None last_name: Optional[str] = None nickname: Optional[str] = None @@ -27,7 +27,7 @@ class PersonCreate(BaseModel): class PersonUpdate(BaseModel): - name: Optional[str] = None + # name is intentionally omitted — always computed from first/last/nickname first_name: Optional[str] = None last_name: Optional[str] = None nickname: Optional[str] = None @@ -55,7 +55,7 @@ class PersonResponse(BaseModel): address: Optional[str] birthday: Optional[date] category: Optional[str] - relationship: Optional[str] + relationship: Optional[str] # deprecated — kept for legacy calendar/birthday compat, will be removed is_favourite: bool company: Optional[str] job_title: Optional[str] diff --git a/frontend/src/components/locations/LocationsPage.tsx b/frontend/src/components/locations/LocationsPage.tsx index 471b5ed..c1941fc 100644 --- a/frontend/src/components/locations/LocationsPage.tsx +++ b/frontend/src/components/locations/LocationsPage.tsx @@ -1,4 +1,4 @@ -import { useState, useMemo, useRef } from 'react'; +import { useState, useMemo, useRef, useEffect } from 'react'; import { Plus, MapPin, Phone, Mail } from 'lucide-react'; import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query'; import { toast } from 'sonner'; @@ -118,6 +118,16 @@ export default function LocationsPage() { setShowForm(true); }; + // Escape key closes detail panel + useEffect(() => { + if (!panelOpen) return; + const handler = (e: KeyboardEvent) => { + if (e.key === 'Escape') setSelectedLocationId(null); + }; + document.addEventListener('keydown', handler); + return () => document.removeEventListener('keydown', handler); + }, [panelOpen]); + const handleCloseForm = () => { setShowForm(false); setEditingLocation(null); diff --git a/frontend/src/components/people/PeoplePage.tsx b/frontend/src/components/people/PeoplePage.tsx index cbbf650..31160fe 100644 --- a/frontend/src/components/people/PeoplePage.tsx +++ b/frontend/src/components/people/PeoplePage.tsx @@ -1,4 +1,4 @@ -import { useState, useMemo, useRef } from 'react'; +import { useState, useMemo, useRef, useEffect } from 'react'; import { Plus, Users, Star, Cake, Phone, Mail, MapPin } from 'lucide-react'; import type { LucideIcon } from 'lucide-react'; import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query'; @@ -285,11 +285,38 @@ export default function PeoplePage() { }, }); + // Escape key closes detail panel + useEffect(() => { + if (!panelOpen) return; + const handler = (e: KeyboardEvent) => { + if (e.key === 'Escape') setSelectedPersonId(null); + }; + document.addEventListener('keydown', handler); + return () => document.removeEventListener('keydown', handler); + }, [panelOpen]); + const handleCloseForm = () => { setShowForm(false); setEditingPerson(null); }; + // Panel header renderer (shared between desktop and mobile) + const renderPersonHeader = (p: Person) => ( +
+
+ {getInitials(p.name)} +
+
+

{p.name}

+ {p.category && ( + {p.category} + )} +
+
+ ); + // Panel getValue const getPanelValue = (p: Person, key: string): string | undefined => { if (key === 'birthday_display' && p.birthday) { @@ -436,21 +463,7 @@ export default function PeoplePage() { onDelete={() => deleteMutation.mutate()} deleteLoading={deleteMutation.isPending} onClose={() => setSelectedPersonId(null)} - renderHeader={(p) => ( -
-
- {getInitials(p.name)} -
-
-

{p.name}

- {p.category && ( - {p.category} - )} -
-
- )} + renderHeader={renderPersonHeader} getUpdatedAt={(p) => p.updated_at} getValue={getPanelValue} /> @@ -472,21 +485,7 @@ export default function PeoplePage() { onDelete={() => deleteMutation.mutate()} deleteLoading={deleteMutation.isPending} onClose={() => setSelectedPersonId(null)} - renderHeader={(p) => ( -
-
- {getInitials(p.name)} -
-
-

{p.name}

- {p.category && ( - {p.category} - )} -
-
- )} + renderHeader={renderPersonHeader} getUpdatedAt={(p) => p.updated_at} getValue={getPanelValue} /> diff --git a/frontend/src/components/shared/CopyableField.tsx b/frontend/src/components/shared/CopyableField.tsx index 7a7c8eb..27f26d2 100644 --- a/frontend/src/components/shared/CopyableField.tsx +++ b/frontend/src/components/shared/CopyableField.tsx @@ -14,6 +14,8 @@ export default function CopyableField({ value, icon: Icon, label }: CopyableFiel navigator.clipboard.writeText(value).then(() => { setCopied(true); setTimeout(() => setCopied(false), 1500); + }).catch(() => { + // Clipboard API can fail in non-secure contexts or when permission is denied }); }; diff --git a/frontend/src/components/shared/utils.ts b/frontend/src/components/shared/utils.ts index 4722b7b..5430fa4 100644 --- a/frontend/src/components/shared/utils.ts +++ b/frontend/src/components/shared/utils.ts @@ -13,6 +13,7 @@ export const avatarColors = [ ]; export function getInitials(name: string): string { + if (!name.trim()) return '??'; const parts = name.trim().split(/\s+/); if (parts.length >= 2) return (parts[0][0] + parts[parts.length - 1][0]).toUpperCase(); return name.slice(0, 2).toUpperCase(); diff --git a/frontend/src/hooks/useTableVisibility.ts b/frontend/src/hooks/useTableVisibility.ts index 1c3b93a..ec0b905 100644 --- a/frontend/src/hooks/useTableVisibility.ts +++ b/frontend/src/hooks/useTableVisibility.ts @@ -2,6 +2,15 @@ import { useState, useEffect, useRef } from 'react'; export type VisibilityMode = 'all' | 'filtered' | 'essential'; +function calculate(width: number, panelOpen: boolean): VisibilityMode { + if (panelOpen) { + return width >= 600 ? 'filtered' : 'essential'; + } + if (width >= 900) return 'all'; + if (width >= 600) return 'filtered'; + return 'essential'; +} + /** * Observes container width via ResizeObserver and returns a visibility mode * for table columns. Adjusts thresholds when a side panel is open. @@ -17,28 +26,18 @@ export function useTableVisibility( const el = containerRef.current; if (!el) return; - const calculate = (width: number): VisibilityMode => { - if (panelOpen) { - return width >= 600 ? 'filtered' : 'essential'; - } - if (width >= 900) return 'all'; - if (width >= 600) return 'filtered'; - return 'essential'; - }; - const observer = new ResizeObserver((entries) => { clearTimeout(timerRef.current); timerRef.current = setTimeout(() => { const entry = entries[0]; if (entry) { - setMode(calculate(entry.contentRect.width)); + setMode(calculate(entry.contentRect.width, panelOpen)); } }, 50); }); observer.observe(el); - // Set initial value - setMode(calculate(el.getBoundingClientRect().width)); + setMode(calculate(el.getBoundingClientRect().width, panelOpen)); return () => { observer.disconnect(); @@ -50,14 +49,7 @@ export function useTableVisibility( useEffect(() => { const el = containerRef.current; if (!el) return; - const width = el.getBoundingClientRect().width; - if (panelOpen) { - setMode(width >= 600 ? 'filtered' : 'essential'); - } else { - if (width >= 900) setMode('all'); - else if (width >= 600) setMode('filtered'); - else setMode('essential'); - } + setMode(calculate(el.getBoundingClientRect().width, panelOpen)); }, [panelOpen, containerRef]); return mode;