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 <noreply@anthropic.com>
This commit is contained in:
Kyle 2026-02-24 21:19:22 +08:00
parent cb9f74a387
commit 8cbc95939a
9 changed files with 82 additions and 62 deletions

View File

@ -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 sqlalchemy.orm import Mapped, mapped_column, relationship
from datetime import datetime from datetime import datetime
from typing import Optional, List from typing import Optional, List
@ -12,7 +12,7 @@ class Location(Base):
name: Mapped[str] = mapped_column(String(255), nullable=False) name: Mapped[str] = mapped_column(String(255), nullable=False)
address: Mapped[str] = mapped_column(Text, nullable=False) address: Mapped[str] = mapped_column(Text, nullable=False)
category: Mapped[str] = mapped_column(String(100), default="other") 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) contact_number: Mapped[Optional[str]] = mapped_column(String(50), nullable=True)
email: Mapped[Optional[str]] = mapped_column(String(255), nullable=True) email: Mapped[Optional[str]] = mapped_column(String(255), nullable=True)
notes: Mapped[Optional[str]] = mapped_column(Text, nullable=True) notes: Mapped[Optional[str]] = mapped_column(Text, nullable=True)

View File

@ -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 sqlalchemy.orm import Mapped, mapped_column, relationship as sa_relationship
from datetime import datetime, date from datetime import datetime, date
from typing import Optional, List from typing import Optional, List
@ -20,7 +20,7 @@ class Person(Base):
first_name: Mapped[Optional[str]] = mapped_column(String(100), nullable=True) first_name: Mapped[Optional[str]] = mapped_column(String(100), nullable=True)
last_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) 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) company: Mapped[Optional[str]] = mapped_column(String(255), nullable=True)
job_title: 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) mobile: Mapped[Optional[str]] = mapped_column(String(50), nullable=True)

View File

@ -1,6 +1,6 @@
from fastapi import APIRouter, Depends, HTTPException, Query from fastapi import APIRouter, Depends, HTTPException, Query
from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy.ext.asyncio import AsyncSession
from sqlalchemy import select from sqlalchemy import select, or_
from datetime import datetime, timezone from datetime import datetime, timezone
from typing import Optional, List from typing import Optional, List
@ -39,7 +39,17 @@ async def get_people(
query = select(Person) query = select(Person)
if search: 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: if category:
query = query.where(Person.category == category) query = query.where(Person.category == category)
@ -58,7 +68,13 @@ async def create_person(
current_user: Settings = Depends(get_current_session) current_user: Settings = Depends(get_current_session)
): ):
"""Create a new person with denormalised display name.""" """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.name = _compute_display_name(
new_person.first_name, new_person.first_name,
new_person.last_name, new_person.last_name,

View File

@ -4,7 +4,7 @@ from typing import Optional
class PersonCreate(BaseModel): 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 first_name: Optional[str] = None
last_name: Optional[str] = None last_name: Optional[str] = None
nickname: Optional[str] = None nickname: Optional[str] = None
@ -27,7 +27,7 @@ class PersonCreate(BaseModel):
class PersonUpdate(BaseModel): class PersonUpdate(BaseModel):
name: Optional[str] = None # name is intentionally omitted — always computed from first/last/nickname
first_name: Optional[str] = None first_name: Optional[str] = None
last_name: Optional[str] = None last_name: Optional[str] = None
nickname: Optional[str] = None nickname: Optional[str] = None
@ -55,7 +55,7 @@ class PersonResponse(BaseModel):
address: Optional[str] address: Optional[str]
birthday: Optional[date] birthday: Optional[date]
category: Optional[str] category: Optional[str]
relationship: Optional[str] relationship: Optional[str] # deprecated — kept for legacy calendar/birthday compat, will be removed
is_favourite: bool is_favourite: bool
company: Optional[str] company: Optional[str]
job_title: Optional[str] job_title: Optional[str]

View File

@ -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 { Plus, MapPin, Phone, Mail } from 'lucide-react';
import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query'; import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query';
import { toast } from 'sonner'; import { toast } from 'sonner';
@ -118,6 +118,16 @@ export default function LocationsPage() {
setShowForm(true); 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 = () => { const handleCloseForm = () => {
setShowForm(false); setShowForm(false);
setEditingLocation(null); setEditingLocation(null);

View File

@ -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 { Plus, Users, Star, Cake, Phone, Mail, MapPin } from 'lucide-react';
import type { LucideIcon } from 'lucide-react'; import type { LucideIcon } from 'lucide-react';
import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query'; 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 = () => { const handleCloseForm = () => {
setShowForm(false); setShowForm(false);
setEditingPerson(null); setEditingPerson(null);
}; };
// Panel header renderer (shared between desktop and mobile)
const renderPersonHeader = (p: Person) => (
<div className="flex items-center gap-3">
<div
className={`h-10 w-10 rounded-full flex items-center justify-center text-sm font-bold shrink-0 ${getAvatarColor(p.name)}`}
>
{getInitials(p.name)}
</div>
<div className="min-w-0">
<h3 className="font-heading text-lg font-semibold truncate">{p.name}</h3>
{p.category && (
<span className="text-xs text-muted-foreground">{p.category}</span>
)}
</div>
</div>
);
// Panel getValue // Panel getValue
const getPanelValue = (p: Person, key: string): string | undefined => { const getPanelValue = (p: Person, key: string): string | undefined => {
if (key === 'birthday_display' && p.birthday) { if (key === 'birthday_display' && p.birthday) {
@ -436,21 +463,7 @@ export default function PeoplePage() {
onDelete={() => deleteMutation.mutate()} onDelete={() => deleteMutation.mutate()}
deleteLoading={deleteMutation.isPending} deleteLoading={deleteMutation.isPending}
onClose={() => setSelectedPersonId(null)} onClose={() => setSelectedPersonId(null)}
renderHeader={(p) => ( renderHeader={renderPersonHeader}
<div className="flex items-center gap-3">
<div
className={`h-10 w-10 rounded-full flex items-center justify-center text-sm font-bold shrink-0 ${getAvatarColor(p.name)}`}
>
{getInitials(p.name)}
</div>
<div className="min-w-0">
<h3 className="font-heading text-lg font-semibold truncate">{p.name}</h3>
{p.category && (
<span className="text-xs text-muted-foreground">{p.category}</span>
)}
</div>
</div>
)}
getUpdatedAt={(p) => p.updated_at} getUpdatedAt={(p) => p.updated_at}
getValue={getPanelValue} getValue={getPanelValue}
/> />
@ -472,21 +485,7 @@ export default function PeoplePage() {
onDelete={() => deleteMutation.mutate()} onDelete={() => deleteMutation.mutate()}
deleteLoading={deleteMutation.isPending} deleteLoading={deleteMutation.isPending}
onClose={() => setSelectedPersonId(null)} onClose={() => setSelectedPersonId(null)}
renderHeader={(p) => ( renderHeader={renderPersonHeader}
<div className="flex items-center gap-3">
<div
className={`h-10 w-10 rounded-full flex items-center justify-center text-sm font-bold shrink-0 ${getAvatarColor(p.name)}`}
>
{getInitials(p.name)}
</div>
<div className="min-w-0">
<h3 className="font-heading text-lg font-semibold truncate">{p.name}</h3>
{p.category && (
<span className="text-xs text-muted-foreground">{p.category}</span>
)}
</div>
</div>
)}
getUpdatedAt={(p) => p.updated_at} getUpdatedAt={(p) => p.updated_at}
getValue={getPanelValue} getValue={getPanelValue}
/> />

View File

@ -14,6 +14,8 @@ export default function CopyableField({ value, icon: Icon, label }: CopyableFiel
navigator.clipboard.writeText(value).then(() => { navigator.clipboard.writeText(value).then(() => {
setCopied(true); setCopied(true);
setTimeout(() => setCopied(false), 1500); setTimeout(() => setCopied(false), 1500);
}).catch(() => {
// Clipboard API can fail in non-secure contexts or when permission is denied
}); });
}; };

View File

@ -13,6 +13,7 @@ export const avatarColors = [
]; ];
export function getInitials(name: string): string { export function getInitials(name: string): string {
if (!name.trim()) return '??';
const parts = name.trim().split(/\s+/); const parts = name.trim().split(/\s+/);
if (parts.length >= 2) return (parts[0][0] + parts[parts.length - 1][0]).toUpperCase(); if (parts.length >= 2) return (parts[0][0] + parts[parts.length - 1][0]).toUpperCase();
return name.slice(0, 2).toUpperCase(); return name.slice(0, 2).toUpperCase();

View File

@ -2,6 +2,15 @@ import { useState, useEffect, useRef } from 'react';
export type VisibilityMode = 'all' | 'filtered' | 'essential'; 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 * Observes container width via ResizeObserver and returns a visibility mode
* for table columns. Adjusts thresholds when a side panel is open. * for table columns. Adjusts thresholds when a side panel is open.
@ -17,28 +26,18 @@ export function useTableVisibility(
const el = containerRef.current; const el = containerRef.current;
if (!el) return; 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) => { const observer = new ResizeObserver((entries) => {
clearTimeout(timerRef.current); clearTimeout(timerRef.current);
timerRef.current = setTimeout(() => { timerRef.current = setTimeout(() => {
const entry = entries[0]; const entry = entries[0];
if (entry) { if (entry) {
setMode(calculate(entry.contentRect.width)); setMode(calculate(entry.contentRect.width, panelOpen));
} }
}, 50); }, 50);
}); });
observer.observe(el); observer.observe(el);
// Set initial value setMode(calculate(el.getBoundingClientRect().width, panelOpen));
setMode(calculate(el.getBoundingClientRect().width));
return () => { return () => {
observer.disconnect(); observer.disconnect();
@ -50,14 +49,7 @@ export function useTableVisibility(
useEffect(() => { useEffect(() => {
const el = containerRef.current; const el = containerRef.current;
if (!el) return; if (!el) return;
const width = el.getBoundingClientRect().width; setMode(calculate(el.getBoundingClientRect().width, panelOpen));
if (panelOpen) {
setMode(width >= 600 ? 'filtered' : 'essential');
} else {
if (width >= 900) setMode('all');
else if (width >= 600) setMode('filtered');
else setMode('essential');
}
}, [panelOpen, containerRef]); }, [panelOpen, containerRef]);
return mode; return mode;