From 8bec2dcc138d33ba137a7947fea17b15bca675ab Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Thu, 2 Apr 2026 16:46:04 +0000 Subject: [PATCH 01/14] feat(relationships): add manual relationship linking for parents, spouses, and children Implements Phase 15.20 - users can now link existing people or create new person stubs as parents, spouses, or children from the PersonDetail page. - Add link-relationship/unlink-relationship API endpoints with validation - Add quick-search endpoint (FTS5 with database scoping) for modal search - Add createPersonStub to id-mapping service for manual person creation - Create RelationshipModal with search + create tabs, debounced search - Add "+ Add" buttons to Parents, Spouses, and Children sections - Replace placeholder modal with functional RelationshipModal --- client/src/components/person/PersonDetail.tsx | 84 ++--- .../components/person/RelationshipModal.tsx | 315 ++++++++++++++++++ client/src/services/api.ts | 32 ++ server/src/routes/person.routes.ts | 215 ++++++++++++ server/src/services/id-mapping.service.ts | 35 ++ 5 files changed, 643 insertions(+), 38 deletions(-) create mode 100644 client/src/components/person/RelationshipModal.tsx diff --git a/client/src/components/person/PersonDetail.tsx b/client/src/components/person/PersonDetail.tsx index d2c48e3a..9e487e51 100644 --- a/client/src/components/person/PersonDetail.tsx +++ b/client/src/components/person/PersonDetail.tsx @@ -14,6 +14,9 @@ import { UploadToFamilySearchDialog } from './UploadToFamilySearchDialog'; import { UploadToAncestryDialog } from './UploadToAncestryDialog'; import { ProviderDataTable } from './ProviderDataTable'; import { LinkPlatformDialog } from './LinkPlatformDialog'; +import { RelationshipModal } from './RelationshipModal'; +import type { RelationshipType } from './RelationshipModal'; + import { PersonAuditIssues } from './PersonAuditIssues'; interface CachedLineage { @@ -190,7 +193,7 @@ export function PersonDetail() { const [syncLoading, setSyncLoading] = useState(false); const [showUploadDialog, setShowUploadDialog] = useState(false); const [showAncestryUploadDialog, setShowAncestryUploadDialog] = useState(false); - const [showRelationshipModal, setShowRelationshipModal] = useState(false); + const [relationshipModalType, setRelationshipModalType] = useState(null); const [hintsProcessing, setHintsProcessing] = useState(false); // Local overrides state @@ -966,9 +969,24 @@ export function PersonDetail() {
{/* Parents */}
-
+
+
Parents +
+ {person.parents.filter(id => id != null).length < 2 && ( + + )}
{person.parents.some(id => id != null) ? (
@@ -999,7 +1017,7 @@ export function PersonDetail() { type="button" className="text-[10px] text-app-accent hover:underline" title="Add or link a spouse" - onClick={() => setShowRelationshipModal(true)} + onClick={() => setRelationshipModalType('spouse')} > + Add @@ -1023,9 +1041,19 @@ export function PersonDetail() { {/* Children */}
-
+
+
Children +
+
{person.children.length > 0 ? (
@@ -1287,40 +1315,20 @@ export function PersonDetail() { loading={linkingLoading} /> - {/* Relationship placeholder modal */} - {showRelationshipModal && ( -
e.target === e.currentTarget && setShowRelationshipModal(false)} - > -
-
-

Add Relationship

- -
-
-

- Coming soon: link existing people or create new profiles for parents, spouses, and children. -

-
- -
-
-
-
- )} + {/* Relationship linking modal */} + setRelationshipModalType(null)} + onLinked={() => { + api.getPerson(dbId!, personId!).then(updated => { + setPerson(updated); + toast.success('Relationship linked'); + }); + }} + />
); } diff --git a/client/src/components/person/RelationshipModal.tsx b/client/src/components/person/RelationshipModal.tsx new file mode 100644 index 00000000..7af8c09b --- /dev/null +++ b/client/src/components/person/RelationshipModal.tsx @@ -0,0 +1,315 @@ +import { useState, useEffect, useRef, useCallback } from 'react'; +import { X, Loader2, Search, UserPlus, Users, Heart, User } from 'lucide-react'; +import toast from 'react-hot-toast'; +import { api } from '../../services/api'; + +export type RelationshipType = 'father' | 'mother' | 'spouse' | 'child'; + +interface RelationshipModalProps { + open: boolean; + dbId: string; + personId: string; + initialType?: RelationshipType; + onClose: () => void; + onLinked: () => void; +} + +interface QuickSearchResult { + personId: string; + displayName: string; + gender: string; + birthName: string | null; + birthYear: number | null; +} + +const TYPE_CONFIG: Record = { + father: { label: 'Father', icon: User, color: 'text-blue-400' }, + mother: { label: 'Mother', icon: User, color: 'text-pink-400' }, + spouse: { label: 'Spouse', icon: Heart, color: 'text-red-400' }, + child: { label: 'Child', icon: Users, color: 'text-green-400' }, +}; + +export function RelationshipModal({ open, dbId, personId, initialType, onClose, onLinked }: RelationshipModalProps) { + const [relType, setRelType] = useState(initialType ?? 'spouse'); + const [mode, setMode] = useState<'search' | 'create'>('search'); + const [query, setQuery] = useState(''); + const [results, setResults] = useState([]); + const [searching, setSearching] = useState(false); + const [linkingId, setLinkingId] = useState(null); + const [newName, setNewName] = useState(''); + const [newGender, setNewGender] = useState<'male' | 'female' | 'unknown'>('unknown'); + const inputRef = useRef(null); + const debounceRef = useRef>(); + + useEffect(() => { + if (open) { + setRelType(initialType ?? 'spouse'); + setMode('search'); + setQuery(''); + setResults([]); + setNewName(''); + setNewGender('unknown'); + setTimeout(() => inputRef.current?.focus(), 100); + } else { + if (debounceRef.current) clearTimeout(debounceRef.current); + } + }, [open, initialType]); + + useEffect(() => { + return () => { + if (debounceRef.current) clearTimeout(debounceRef.current); + }; + }, []); + + useEffect(() => { + if (relType === 'father') setNewGender('male'); + else if (relType === 'mother') setNewGender('female'); + else setNewGender('unknown'); + }, [relType]); + + const doSearch = useCallback(async (q: string) => { + if (q.length < 2) { + setResults([]); + return; + } + setSearching(true); + const data = await api.quickSearchPersons(dbId, q); + setResults(data.filter(r => r.personId !== personId)); + setSearching(false); + }, [dbId, personId]); + + const handleQueryChange = (value: string) => { + setQuery(value); + if (debounceRef.current) clearTimeout(debounceRef.current); + debounceRef.current = setTimeout(() => doSearch(value), 300); + }; + + const handleLinkExisting = async (targetId: string) => { + setLinkingId(targetId); + const result = await api.linkRelationship(dbId, personId, relType, targetId).catch(err => { + toast.error(err.message || 'Failed to link'); + return null; + }); + setLinkingId(null); + if (!result) return; + onLinked(); + onClose(); + }; + + const handleCreateNew = async (e: React.FormEvent) => { + e.preventDefault(); + if (!newName.trim()) return; + setLinkingId('new'); + const result = await api.linkRelationship(dbId, personId, relType, undefined, { name: newName.trim(), gender: newGender }).catch(err => { + toast.error(err.message || 'Failed to create'); + return null; + }); + setLinkingId(null); + if (!result) return; + onLinked(); + onClose(); + }; + + if (!open) return null; + + const linking = linkingId !== null; + + return ( +
e.target === e.currentTarget && onClose()} + > +
+
+
+ +

Add Relationship

+
+ +
+ +
+
+ {(Object.keys(TYPE_CONFIG) as RelationshipType[]).map(type => { + const cfg = TYPE_CONFIG[type]; + const Icon = cfg.icon; + return ( + + ); + })} +
+
+ +
+
+ + +
+
+ +
+ {mode === 'search' ? ( +
+
+ + handleQueryChange(e.target.value)} + placeholder="Search by name..." + className="w-full pl-8 pr-3 py-2 bg-app-bg border border-app-border rounded text-app-text text-sm focus:outline-none focus:ring-2 focus:ring-app-accent" + disabled={linking} + /> + {searching && } +
+ + {results.length > 0 && ( +
+ {results.map(r => ( + + ))} +
+ )} + + {query.length >= 2 && !searching && results.length === 0 && ( +

+ No matching people found.{' '} + +

+ )} + + {query.length < 2 && ( +

+ Type at least 2 characters to search +

+ )} +
+ ) : ( +
+
+ + setNewName(e.target.value)} + placeholder="e.g. John Smith" + className="w-full px-3 py-2 bg-app-bg border border-app-border rounded text-app-text text-sm focus:outline-none focus:ring-2 focus:ring-app-accent" + disabled={linking} + /> +
+ +
+ +
+ {(['male', 'female', 'unknown'] as const).map(g => ( + + ))} +
+
+ +
+ + +
+
+ )} +
+
+
+ ); +} diff --git a/client/src/services/api.ts b/client/src/services/api.ts index 0c61dade..e2258a7f 100644 --- a/client/src/services/api.ts +++ b/client/src/services/api.ts @@ -704,6 +704,38 @@ export const api = { } ), + // Relationship linking + quickSearchPersons: (dbId: string, q: string) => + fetchJson>(`/persons/${dbId}/quick-search?q=${encodeURIComponent(q)}`), + + linkRelationship: (dbId: string, personId: string, relationshipType: string, targetId?: string, newPerson?: { name: string; gender?: string }) => + fetchJson<{ + personId: string; + targetId: string; + relationshipType: string; + createdNew: boolean; + }>( + `/persons/${dbId}/${personId}/link-relationship`, + { + method: 'POST', + body: JSON.stringify({ relationshipType, targetId, newPerson }) + } + ), + + unlinkRelationship: (dbId: string, personId: string, relationshipType: string, targetId: string) => + fetchJson<{ personId: string; targetId: string; relationshipType: string }>( + `/persons/${dbId}/${personId}/unlink-relationship`, + { + method: 'DELETE', + body: JSON.stringify({ relationshipType, targetId }) + } + ), // AI Discovery quickDiscovery: (dbId: string, sampleSize = 100, options?: { model?: string; excludeBiblical?: boolean; minBirthYear?: number; maxGenerations?: number; customPrompt?: string }) => diff --git a/server/src/routes/person.routes.ts b/server/src/routes/person.routes.ts index c807bef0..d3d83c5a 100644 --- a/server/src/routes/person.routes.ts +++ b/server/src/routes/person.routes.ts @@ -14,6 +14,9 @@ import { logger } from '../lib/logger.js'; import type { BuiltInProvider } from '@fsf/shared'; import { PHOTOS_DIR, PROVIDER_CACHE_DIR } from '../utils/paths.js'; import { resolveCanonicalOrFail } from '../utils/resolveCanonical.js'; +import { sanitizeFtsQuery } from '../utils/validation.js'; + +const VALID_RELATIONSHIP_TYPES = ['father', 'mother', 'spouse', 'child'] as const; export const personRoutes = Router(); @@ -25,6 +28,51 @@ personRoutes.get('/:dbId', async (req, res, next) => { if (result) res.json({ success: true, data: result }); }); +// GET /api/persons/:dbId/quick-search?q=name +// Must be registered before /:dbId/:personId to avoid route conflict +personRoutes.get('/:dbId/quick-search', async (req, res, next) => { + const q = (req.query.q as string || '').trim(); + if (!q || q.length < 2) { + return res.json({ success: true, data: [] }); + } + + if (!databaseService.isSqliteEnabled()) { + return res.json({ success: true, data: [] }); + } + + const { dbId } = req.params; + const sanitized = sanitizeFtsQuery(q); + if (!sanitized) return res.json({ success: true, data: [] }); + const ftsQuery = `"${sanitized}"*`; + + const results = sqliteService.queryAll<{ + person_id: string; + display_name: string; + gender: string; + birth_name: string | null; + birth_year: number | null; + }>( + `SELECT p.person_id, p.display_name, p.gender, p.birth_name, ve.date_year AS birth_year + FROM person p + JOIN database_membership dm ON p.person_id = dm.person_id + LEFT JOIN vital_event ve ON ve.person_id = p.person_id AND ve.event_type = 'birth' + WHERE dm.db_id = @dbId + AND p.person_id IN (SELECT person_id FROM person_fts WHERE person_fts MATCH @q) + LIMIT 20`, + { dbId, q: ftsQuery } + ); + + const data = results.map(r => ({ + personId: r.person_id, + displayName: r.display_name, + gender: r.gender, + birthName: r.birth_name, + birthYear: r.birth_year ?? null, + })); + + res.json({ success: true, data }); +}); + // GET /api/persons/:dbId/:personId - Get single person personRoutes.get('/:dbId/:personId', async (req, res, next) => { // Services handle ID resolution internally (accepts both canonical ULID and external IDs) @@ -653,3 +701,170 @@ personRoutes.put('/:dbId/:personId/use-field', async (req, res, next) => { data: override }); }); + +// POST /api/persons/:dbId/:personId/link-relationship +// Link an existing person or create a new stub as parent/spouse/child +// Body: { relationshipType: 'father'|'mother'|'spouse'|'child', targetId?: string, newPerson?: { name: string, gender?: string } } +personRoutes.post('/:dbId/:personId/link-relationship', async (req, res, next) => { + const { personId } = req.params; + const { relationshipType, targetId, newPerson } = req.body; + + if (!relationshipType || !VALID_RELATIONSHIP_TYPES.includes(relationshipType)) { + return res.status(400).json({ success: false, error: `Invalid relationshipType. Must be one of: ${VALID_RELATIONSHIP_TYPES.join(', ')}` }); + } + + if (!targetId && !newPerson?.name) { + return res.status(400).json({ success: false, error: 'Provide either targetId (existing person) or newPerson.name (to create a stub)' }); + } + + const canonical = resolveCanonicalOrFail(personId, res); + if (!canonical) return; + + if (!databaseService.isSqliteEnabled()) { + return res.status(400).json({ success: false, error: 'SQLite must be enabled for relationship linking' }); + } + + // Resolve or create the target person + let resolvedTargetId = targetId; + let createdNew = false; + + if (targetId) { + // Verify target person exists + const existing = sqliteService.queryOne<{ person_id: string }>( + 'SELECT person_id FROM person WHERE person_id = @id', + { id: targetId } + ); + if (!existing) { + return res.status(404).json({ success: false, error: 'Target person not found' }); + } + } else { + // Create a new person stub + const gender = newPerson.gender || (relationshipType === 'father' ? 'male' : relationshipType === 'mother' ? 'female' : 'unknown'); + resolvedTargetId = idMappingService.createPersonStub(newPerson.name, { gender }); + createdNew = true; + logger.done('link-relationship', `Created person stub: ${newPerson.name} (${resolvedTargetId})`); + } + + // Prevent self-linking + if (resolvedTargetId === canonical) { + return res.status(400).json({ success: false, error: 'Cannot link a person to themselves' }); + } + + // Create the appropriate edge + if (relationshipType === 'father' || relationshipType === 'mother') { + // Check for duplicate + const existing = sqliteService.queryOne<{ id: number }>( + 'SELECT id FROM parent_edge WHERE child_id = @childId AND parent_id = @parentId', + { childId: canonical, parentId: resolvedTargetId } + ); + if (existing) { + return res.status(409).json({ success: false, error: 'This parent relationship already exists' }); + } + + sqliteService.run( + `INSERT INTO parent_edge (child_id, parent_id, parent_role, source, confidence) + VALUES (@childId, @parentId, @role, 'manual', 1.0)`, + { childId: canonical, parentId: resolvedTargetId, role: relationshipType } + ); + } else if (relationshipType === 'spouse') { + // Normalize ordering (smaller ID first) to prevent duplicate pairs + const [p1, p2] = canonical < resolvedTargetId! ? [canonical, resolvedTargetId] : [resolvedTargetId, canonical]; + const existing = sqliteService.queryOne<{ id: number }>( + 'SELECT id FROM spouse_edge WHERE person1_id = @p1 AND person2_id = @p2', + { p1, p2 } + ); + if (existing) { + return res.status(409).json({ success: false, error: 'This spouse relationship already exists' }); + } + + sqliteService.run( + `INSERT INTO spouse_edge (person1_id, person2_id, source, confidence) + VALUES (@p1, @p2, 'manual', 1.0)`, + { p1, p2 } + ); + } else if (relationshipType === 'child') { + // The current person is the parent, target is the child + const existing = sqliteService.queryOne<{ id: number }>( + 'SELECT id FROM parent_edge WHERE child_id = @childId AND parent_id = @parentId', + { childId: resolvedTargetId, parentId: canonical } + ); + if (existing) { + return res.status(409).json({ success: false, error: 'This child relationship already exists' }); + } + + // Determine parent role from current person's gender + const row = sqliteService.queryOne<{ gender: string }>( + 'SELECT gender FROM person WHERE person_id = @id', + { id: canonical } + ); + const parentRole = row?.gender === 'female' ? 'mother' : row?.gender === 'male' ? 'father' : 'parent'; + + sqliteService.run( + `INSERT INTO parent_edge (child_id, parent_id, parent_role, source, confidence) + VALUES (@childId, @parentId, @role, 'manual', 1.0)`, + { childId: resolvedTargetId, parentId: canonical, role: parentRole } + ); + } + + logger.done('link-relationship', `Linked ${relationshipType}: ${canonical} ↔ ${resolvedTargetId}`); + + res.json({ + success: true, + data: { + personId: canonical, + targetId: resolvedTargetId, + relationshipType, + createdNew, + } + }); +}); + +// DELETE /api/persons/:dbId/:personId/unlink-relationship +// Remove a relationship between two people +// Body: { relationshipType: 'father'|'mother'|'spouse'|'child', targetId: string } +personRoutes.delete('/:dbId/:personId/unlink-relationship', async (req, res, next) => { + const { personId } = req.params; + const { relationshipType, targetId } = req.body; + + if (!relationshipType || !VALID_RELATIONSHIP_TYPES.includes(relationshipType) || !targetId) { + return res.status(400).json({ success: false, error: 'relationshipType and targetId are required' }); + } + + const canonical = resolveCanonicalOrFail(personId, res); + if (!canonical) return; + + if (!databaseService.isSqliteEnabled()) { + return res.status(400).json({ success: false, error: 'SQLite must be enabled' }); + } + + let deleted = false; + + if (relationshipType === 'father' || relationshipType === 'mother') { + const result = sqliteService.run( + 'DELETE FROM parent_edge WHERE child_id = @childId AND parent_id = @parentId', + { childId: canonical, parentId: targetId } + ); + deleted = result.changes > 0; + } else if (relationshipType === 'spouse') { + const result = sqliteService.run( + 'DELETE FROM spouse_edge WHERE (person1_id = @a AND person2_id = @b) OR (person1_id = @b AND person2_id = @a)', + { a: canonical, b: targetId } + ); + deleted = result.changes > 0; + } else if (relationshipType === 'child') { + const result = sqliteService.run( + 'DELETE FROM parent_edge WHERE child_id = @childId AND parent_id = @parentId', + { childId: targetId, parentId: canonical } + ); + deleted = result.changes > 0; + } + + if (!deleted) { + return res.status(404).json({ success: false, error: 'Relationship not found' }); + } + + logger.done('unlink-relationship', `Unlinked ${relationshipType}: ${canonical} ↔ ${targetId}`); + + res.json({ success: true, data: { personId: canonical, targetId, relationshipType } }); +}); + diff --git a/server/src/services/id-mapping.service.ts b/server/src/services/id-mapping.service.ts index 876801a0..25f610a4 100644 --- a/server/src/services/id-mapping.service.ts +++ b/server/src/services/id-mapping.service.ts @@ -162,6 +162,40 @@ function createPerson( return personId; } +/** + * Create a minimal person record (stub) without an external identity. + * Used for manually linking family members who aren't in any provider yet. + */ +function createPersonStub( + displayName: string, + options?: { + birthName?: string; + gender?: 'male' | 'female' | 'unknown'; + living?: boolean; + bio?: string; + } +): string { + const personId = ulid(); + + sqliteService.run( + `INSERT INTO person (person_id, display_name, birth_name, gender, living, bio) + VALUES (@personId, @displayName, @birthName, @gender, @living, @bio)`, + { + personId, + displayName, + birthName: options?.birthName ?? null, + gender: options?.gender ?? 'unknown', + living: options?.living ? 1 : 0, + bio: options?.bio ?? null, + } + ); + + // Update FTS index + sqliteService.updatePersonFts(personId, displayName, options?.birthName); + + return personId; +} + /** * Register an external ID for an existing person */ @@ -353,6 +387,7 @@ export const idMappingService = { getExternalIds, getExternalId, createPerson, + createPersonStub, registerExternalId, removeExternalId, getOrCreateCanonicalId, From 84544a4f4b37e9171131a0661b40374277bf9a49 Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Thu, 2 Apr 2026 16:46:19 +0000 Subject: [PATCH 02/14] docs: mark Phase 15.20 (relationship linking) as complete --- PLAN.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/PLAN.md b/PLAN.md index 686acec9..c4f8d6dd 100644 --- a/PLAN.md +++ b/PLAN.md @@ -36,7 +36,7 @@ High-level project roadmap. For detailed phase documentation, see [docs/roadmap. | 15.17 | Data integrity + bulk discovery | ✅ | | 15.18 | Separate provider download from auto-apply | ✅ | | 15.19 | Normalize FamilySearch as downstream provider | ✅ | -| 15.20 | Relationship linking (parents, spouses, children) | 📋 | +| 15.20 | Relationship linking (parents, spouses, children) | ✅ | | 15.22 | Ancestry free hints automation | ✅ | | 15.23 | Migration Map visualization | ✅ | | 16 | Multi-platform sync architecture | 📋 | From c182970b4117cb8cb0844ef50491112359c0c8b4 Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Mon, 6 Apr 2026 21:28:42 -0700 Subject: [PATCH 03/14] fix(relationships): address PR #70 review feedback Server fixes: - quick-search: replace LEFT JOIN vital_event with subquery (MIN+GROUP BY) to prevent duplicate rows when persons have multiple birth records - link-relationship: scope source/target to dbId, reject cross-database links, add new stubs to database_membership in same transaction - link-relationship: validate gender against CHECK constraint values (male/female/unknown), coerce based on relationshipType when invalid - link-relationship: wrap stub creation + membership + edge insertion in a single transaction so failures roll back all writes - link-relationship: pre-check duplicates before any writes; extract checkDuplicateEdge helper - link/unlink-relationship: validate targetId as canonical ULID - unlink-relationship: scope deletes via membership pre-checks; remove redundant EXISTS guards in DELETE statements for consistency - createPersonStub: wrap insert + FTS update in transaction - extract isPersonInDatabase helper shared by link/unlink Client fixes: - RelationshipModal: handle quickSearchPersons errors with toast, ensure searching state always clears via finally - RelationshipModal: reset searching/linkingId on modal open so a re-opened modal never shows stale spinner - RelationshipModal: drop unused color field from TYPE_CONFIG - api.ts: type linkRelationship/unlinkRelationship as RelationshipType instead of string; move type to client/src/types/relationship.ts - PersonDetail: extract reloadPerson helper; refresh parent/spouse/child data after linking instead of only updating the person object - PersonDetail: handle errors in onLinked with toast feedback - PersonDetail: determine missing parent role from gender, not array index (parents.filter(Boolean) collapses sparse positions) --- client/src/components/person/PersonDetail.tsx | 160 ++++++++------ .../components/person/RelationshipModal.tsx | 27 ++- client/src/services/api.ts | 5 +- client/src/types/relationship.ts | 1 + server/src/routes/person.routes.ts | 206 ++++++++++++------ server/src/services/id-mapping.service.ts | 30 +-- 6 files changed, 274 insertions(+), 155 deletions(-) create mode 100644 client/src/types/relationship.ts diff --git a/client/src/components/person/PersonDetail.tsx b/client/src/components/person/PersonDetail.tsx index 9e487e51..c0efc007 100644 --- a/client/src/components/person/PersonDetail.tsx +++ b/client/src/components/person/PersonDetail.tsx @@ -204,6 +204,81 @@ export function PersonDetail() { const [addingAlias, setAddingAlias] = useState(false); const [addingOccupation, setAddingOccupation] = useState(false); + // Refreshes person + all family data (parents, spouses, children, family photos). + // Called on initial load and after a relationship is linked. + const reloadPerson = useCallback(async (signal?: AbortSignal) => { + if (!dbId || !personId) return; + + const personData = await api.getPerson(dbId, personId); + if (signal?.aborted) return; + setPerson(personData); + + const validParentIds = personData.parents.filter((id): id is string => id != null); + const allFamilyIds: string[] = [ + ...validParentIds, + ...(personData.spouses || []), + ...personData.children, + ]; + + // Fetch parent data + if (validParentIds.length > 0) { + const parentResults = await Promise.all( + validParentIds.map((pid: string) => api.getPerson(dbId, pid).catch(() => null)) + ); + if (signal?.aborted) return; + const parents: Record = {}; + parentResults.forEach((p: PersonWithId | null, idx: number) => { + if (p) parents[validParentIds[idx]] = p; + }); + setParentData(parents); + } else { + setParentData({}); + } + + // Fetch spouse data + if (personData.spouses && personData.spouses.length > 0) { + const spouseResults = await Promise.all( + personData.spouses.map((sid: string) => api.getPerson(dbId, sid).catch(() => null)) + ); + if (signal?.aborted) return; + const spouses: Record = {}; + spouseResults.forEach((s: PersonWithId | null, idx: number) => { + if (s && personData.spouses) spouses[personData.spouses[idx]] = s; + }); + setSpouseData(spouses); + } else { + setSpouseData({}); + } + + // Fetch children data + if (personData.children.length > 0) { + const childResults = await Promise.all( + personData.children.map((cid: string) => api.getPerson(dbId, cid).catch(() => null)) + ); + if (signal?.aborted) return; + const children: Record = {}; + childResults.forEach((c: PersonWithId | null, idx: number) => { + if (c) children[personData.children[idx]] = c; + }); + setChildData(children); + } else { + setChildData({}); + } + + // Check photos for all family members (batch) + if (allFamilyIds.length > 0) { + const photoChecks = await Promise.all( + allFamilyIds.map((id: string) => api.hasPhoto(id).then(r => ({ id, exists: r?.exists ?? false })).catch(() => ({ id, exists: false }))) + ); + if (signal?.aborted) return; + const photos: Record = {}; + photoChecks.forEach(({ id, exists }) => { photos[id] = exists; }); + setFamilyPhotos(photos); + } else { + setFamilyPhotos({}); + } + }, [dbId, personId]); + useEffect(() => { if (!dbId || !personId) return; @@ -243,7 +318,6 @@ export function PersonDetail() { }).catch(() => []); Promise.all([ - api.getPerson(dbId, personId), api.getDatabase(dbId), api.getScrapedData(personId).catch(() => null), api.hasPhoto(personId).catch(() => ({ exists: false })), @@ -253,10 +327,9 @@ export function PersonDetail() { api.hasWikiTreePhoto(personId).catch(() => ({ exists: false })), api.hasLinkedInPhoto(personId).catch(() => ({ exists: false })), ]) - .then(async ([personData, dbData, _scraped, photoCheck, augment, wikiPhotoCheck, ancestryPhotoCheck, wikiTreePhotoCheck, linkedInPhotoCheck]) => { + .then(async ([dbData, _scraped, photoCheck, augment, wikiPhotoCheck, ancestryPhotoCheck, wikiTreePhotoCheck, linkedInPhotoCheck]) => { if (signal.aborted) return; - setPerson(personData); setDatabase(dbData); setPhotoStatus({ primary: photoCheck?.exists ?? false, @@ -268,63 +341,9 @@ export function PersonDetail() { }); setAugmentation(augment); - // Collect all family member IDs for batch photo check - const validParentIds = personData.parents.filter((id): id is string => id != null); - const allFamilyIds: string[] = [ - ...validParentIds, - ...(personData.spouses || []), - ...personData.children, - ]; - - // Fetch parent data - if (validParentIds.length > 0) { - const parentResults = await Promise.all( - validParentIds.map((pid: string) => api.getPerson(dbId, pid).catch(() => null)) - ); - if (signal.aborted) return; - const parents: Record = {}; - parentResults.forEach((p: PersonWithId | null, idx: number) => { - if (p) parents[validParentIds[idx]] = p; - }); - setParentData(parents); - } - - // Fetch spouse data - if (personData.spouses && personData.spouses.length > 0) { - const spouseResults = await Promise.all( - personData.spouses.map((sid: string) => api.getPerson(dbId, sid).catch(() => null)) - ); - if (signal.aborted) return; - const spouses: Record = {}; - spouseResults.forEach((s: PersonWithId | null, idx: number) => { - if (s && personData.spouses) spouses[personData.spouses[idx]] = s; - }); - setSpouseData(spouses); - } - - // Fetch children data - if (personData.children.length > 0) { - const childResults = await Promise.all( - personData.children.map((cid: string) => api.getPerson(dbId, cid).catch(() => null)) - ); - if (signal.aborted) return; - const children: Record = {}; - childResults.forEach((c: PersonWithId | null, idx: number) => { - if (c) children[personData.children[idx]] = c; - }); - setChildData(children); - } - - // Check photos for all family members (batch) - if (allFamilyIds.length > 0) { - const photoChecks = await Promise.all( - allFamilyIds.map((id: string) => api.hasPhoto(id).then(r => ({ id, exists: r?.exists ?? false })).catch(() => ({ id, exists: false }))) - ); - if (signal.aborted) return; - const photos: Record = {}; - photoChecks.forEach(({ id, exists }) => { photos[id] = exists; }); - setFamilyPhotos(photos); - } + // Load person + family data via shared helper + await reloadPerson(signal); + if (signal.aborted) return; // Check for cached lineage const cached = getCachedLineage(dbId, personId); @@ -341,7 +360,7 @@ export function PersonDetail() { }); return () => controller.abort(); - }, [dbId, personId]); + }, [dbId, personId, reloadPerson]); const calculateLineage = async () => { if (!dbId || !personId || !database?.rootId) return; @@ -980,7 +999,14 @@ export function PersonDetail() { className="text-[10px] text-app-accent hover:underline" title="Add or link a parent" onClick={() => { - const hasFather = person.parents[0] != null; + // Determine which parent role is missing by checking the + // genders of existing parents (parents.filter(Boolean) on + // the server collapses sparse positions, so index alone + // is unreliable). + const existingGenders = person.parents + .filter((id): id is string => id != null) + .map(id => parentData[id]?.gender); + const hasFather = existingGenders.includes('male'); setRelationshipModalType(hasFather ? 'mother' : 'father'); }} > @@ -1322,11 +1348,13 @@ export function PersonDetail() { personId={personId!} initialType={relationshipModalType ?? undefined} onClose={() => setRelationshipModalType(null)} - onLinked={() => { - api.getPerson(dbId!, personId!).then(updated => { - setPerson(updated); + onLinked={async () => { + try { + await reloadPerson(); toast.success('Relationship linked'); - }); + } catch (error) { + toast.error('Failed to refresh person after linking relationship'); + } }} />
diff --git a/client/src/components/person/RelationshipModal.tsx b/client/src/components/person/RelationshipModal.tsx index 7af8c09b..bb769fde 100644 --- a/client/src/components/person/RelationshipModal.tsx +++ b/client/src/components/person/RelationshipModal.tsx @@ -2,8 +2,9 @@ import { useState, useEffect, useRef, useCallback } from 'react'; import { X, Loader2, Search, UserPlus, Users, Heart, User } from 'lucide-react'; import toast from 'react-hot-toast'; import { api } from '../../services/api'; +import type { RelationshipType } from '../../types/relationship'; -export type RelationshipType = 'father' | 'mother' | 'spouse' | 'child'; +export type { RelationshipType }; interface RelationshipModalProps { open: boolean; @@ -22,11 +23,11 @@ interface QuickSearchResult { birthYear: number | null; } -const TYPE_CONFIG: Record = { - father: { label: 'Father', icon: User, color: 'text-blue-400' }, - mother: { label: 'Mother', icon: User, color: 'text-pink-400' }, - spouse: { label: 'Spouse', icon: Heart, color: 'text-red-400' }, - child: { label: 'Child', icon: Users, color: 'text-green-400' }, +const TYPE_CONFIG: Record = { + father: { label: 'Father', icon: User }, + mother: { label: 'Mother', icon: User }, + spouse: { label: 'Spouse', icon: Heart }, + child: { label: 'Child', icon: Users }, }; export function RelationshipModal({ open, dbId, personId, initialType, onClose, onLinked }: RelationshipModalProps) { @@ -47,6 +48,8 @@ export function RelationshipModal({ open, dbId, personId, initialType, onClose, setMode('search'); setQuery(''); setResults([]); + setSearching(false); + setLinkingId(null); setNewName(''); setNewGender('unknown'); setTimeout(() => inputRef.current?.focus(), 100); @@ -73,9 +76,15 @@ export function RelationshipModal({ open, dbId, personId, initialType, onClose, return; } setSearching(true); - const data = await api.quickSearchPersons(dbId, q); - setResults(data.filter(r => r.personId !== personId)); - setSearching(false); + try { + const data = await api.quickSearchPersons(dbId, q); + setResults(data.filter(r => r.personId !== personId)); + } catch (error) { + console.error('Failed to search persons', error); + toast.error('Failed to search persons. Please try again.'); + } finally { + setSearching(false); + } }, [dbId, personId]); const handleQueryChange = (value: string) => { diff --git a/client/src/services/api.ts b/client/src/services/api.ts index e2258a7f..fcaaefc8 100644 --- a/client/src/services/api.ts +++ b/client/src/services/api.ts @@ -1,3 +1,4 @@ +import type { RelationshipType } from '../types/relationship'; import type { DatabaseInfo, PersonWithId, @@ -714,7 +715,7 @@ export const api = { birthYear: number | null; }>>(`/persons/${dbId}/quick-search?q=${encodeURIComponent(q)}`), - linkRelationship: (dbId: string, personId: string, relationshipType: string, targetId?: string, newPerson?: { name: string; gender?: string }) => + linkRelationship: (dbId: string, personId: string, relationshipType: RelationshipType, targetId?: string, newPerson?: { name: string; gender?: string }) => fetchJson<{ personId: string; targetId: string; @@ -728,7 +729,7 @@ export const api = { } ), - unlinkRelationship: (dbId: string, personId: string, relationshipType: string, targetId: string) => + unlinkRelationship: (dbId: string, personId: string, relationshipType: RelationshipType, targetId: string) => fetchJson<{ personId: string; targetId: string; relationshipType: string }>( `/persons/${dbId}/${personId}/unlink-relationship`, { diff --git a/client/src/types/relationship.ts b/client/src/types/relationship.ts new file mode 100644 index 00000000..d68cca2c --- /dev/null +++ b/client/src/types/relationship.ts @@ -0,0 +1 @@ +export type RelationshipType = 'father' | 'mother' | 'spouse' | 'child'; diff --git a/server/src/routes/person.routes.ts b/server/src/routes/person.routes.ts index d3d83c5a..fcf7601a 100644 --- a/server/src/routes/person.routes.ts +++ b/server/src/routes/person.routes.ts @@ -14,7 +14,7 @@ import { logger } from '../lib/logger.js'; import type { BuiltInProvider } from '@fsf/shared'; import { PHOTOS_DIR, PROVIDER_CACHE_DIR } from '../utils/paths.js'; import { resolveCanonicalOrFail } from '../utils/resolveCanonical.js'; -import { sanitizeFtsQuery } from '../utils/validation.js'; +import { sanitizeFtsQuery, isCanonicalId } from '../utils/validation.js'; const VALID_RELATIONSHIP_TYPES = ['father', 'mother', 'spouse', 'child'] as const; @@ -52,10 +52,15 @@ personRoutes.get('/:dbId/quick-search', async (req, res, next) => { birth_name: string | null; birth_year: number | null; }>( - `SELECT p.person_id, p.display_name, p.gender, p.birth_name, ve.date_year AS birth_year + `SELECT p.person_id, p.display_name, p.gender, p.birth_name, ve.birth_year FROM person p JOIN database_membership dm ON p.person_id = dm.person_id - LEFT JOIN vital_event ve ON ve.person_id = p.person_id AND ve.event_type = 'birth' + LEFT JOIN ( + SELECT person_id, MIN(date_year) AS birth_year + FROM vital_event + WHERE event_type = 'birth' + GROUP BY person_id + ) ve ON ve.person_id = p.person_id WHERE dm.db_id = @dbId AND p.person_id IN (SELECT person_id FROM person_fts WHERE person_fts MATCH @q) LIMIT 20`, @@ -702,11 +707,22 @@ personRoutes.put('/:dbId/:personId/use-field', async (req, res, next) => { }); }); +/** + * Check whether a person belongs to a given database. + * Shared by link-relationship and unlink-relationship to prevent cross-database modifications. + */ +function isPersonInDatabase(personId: string, dbId: string): boolean { + return !!sqliteService.queryOne<{ person_id: string }>( + 'SELECT person_id FROM database_membership WHERE db_id = @dbId AND person_id = @personId', + { dbId, personId } + ); +} + // POST /api/persons/:dbId/:personId/link-relationship // Link an existing person or create a new stub as parent/spouse/child // Body: { relationshipType: 'father'|'mother'|'spouse'|'child', targetId?: string, newPerson?: { name: string, gender?: string } } personRoutes.post('/:dbId/:personId/link-relationship', async (req, res, next) => { - const { personId } = req.params; + const { dbId, personId } = req.params; const { relationshipType, targetId, newPerson } = req.body; if (!relationshipType || !VALID_RELATIONSHIP_TYPES.includes(relationshipType)) { @@ -724,12 +740,24 @@ personRoutes.post('/:dbId/:personId/link-relationship', async (req, res, next) = return res.status(400).json({ success: false, error: 'SQLite must be enabled for relationship linking' }); } - // Resolve or create the target person - let resolvedTargetId = targetId; + // Verify the source person belongs to this database + if (!isPersonInDatabase(canonical, dbId)) { + return res.status(403).json({ success: false, error: 'Person does not belong to the specified database' }); + } + + // Validate targetId format and existence + duplicate checks BEFORE any writes, + // so 4xx responses don't leave behind orphaned stubs. let createdNew = false; + let resolvedTargetId: string; + let stubGender: 'male' | 'female' | 'unknown' = 'unknown'; if (targetId) { - // Verify target person exists + if (!isCanonicalId(targetId)) { + return res.status(400).json({ success: false, error: 'Invalid targetId format' }); + } + if (targetId === canonical) { + return res.status(400).json({ success: false, error: 'Cannot link a person to themselves' }); + } const existing = sqliteService.queryOne<{ person_id: string }>( 'SELECT person_id FROM person WHERE person_id = @id', { id: targetId } @@ -737,75 +765,76 @@ personRoutes.post('/:dbId/:personId/link-relationship', async (req, res, next) = if (!existing) { return res.status(404).json({ success: false, error: 'Target person not found' }); } + resolvedTargetId = targetId; + + // Pre-check duplicate edges. Stubs can never collide so this only applies here. + const dupError = checkDuplicateEdge(canonical, resolvedTargetId, relationshipType); + if (dupError) { + return res.status(409).json({ success: false, error: dupError }); + } } else { - // Create a new person stub - const gender = newPerson.gender || (relationshipType === 'father' ? 'male' : relationshipType === 'mother' ? 'female' : 'unknown'); - resolvedTargetId = idMappingService.createPersonStub(newPerson.name, { gender }); + // Validate and coerce gender to the DB CHECK constraint values + const requestedGender = typeof newPerson.gender === 'string' ? newPerson.gender.toLowerCase() : ''; + stubGender = + requestedGender === 'male' || requestedGender === 'female' || requestedGender === 'unknown' + ? requestedGender + : relationshipType === 'father' + ? 'male' + : relationshipType === 'mother' + ? 'female' + : 'unknown'; createdNew = true; - logger.done('link-relationship', `Created person stub: ${newPerson.name} (${resolvedTargetId})`); + resolvedTargetId = ''; // assigned inside the transaction below } - // Prevent self-linking - if (resolvedTargetId === canonical) { - return res.status(400).json({ success: false, error: 'Cannot link a person to themselves' }); + // For child links, look up parent role from current person's gender (read-only) + let childParentRole = 'parent'; + if (relationshipType === 'child') { + const row = sqliteService.queryOne<{ gender: string }>( + 'SELECT gender FROM person WHERE person_id = @id', + { id: canonical } + ); + childParentRole = row?.gender === 'female' ? 'mother' : row?.gender === 'male' ? 'father' : 'parent'; } - // Create the appropriate edge - if (relationshipType === 'father' || relationshipType === 'mother') { - // Check for duplicate - const existing = sqliteService.queryOne<{ id: number }>( - 'SELECT id FROM parent_edge WHERE child_id = @childId AND parent_id = @parentId', - { childId: canonical, parentId: resolvedTargetId } - ); - if (existing) { - return res.status(409).json({ success: false, error: 'This parent relationship already exists' }); + // Single transaction wraps stub creation + membership + edge insertion so a + // failure anywhere rolls back all writes (no orphaned stubs or memberships). + sqliteService.transaction(() => { + if (createdNew) { + resolvedTargetId = idMappingService.createPersonStub(newPerson.name, { gender: stubGender }); } sqliteService.run( - `INSERT INTO parent_edge (child_id, parent_id, parent_role, source, confidence) - VALUES (@childId, @parentId, @role, 'manual', 1.0)`, - { childId: canonical, parentId: resolvedTargetId, role: relationshipType } - ); - } else if (relationshipType === 'spouse') { - // Normalize ordering (smaller ID first) to prevent duplicate pairs - const [p1, p2] = canonical < resolvedTargetId! ? [canonical, resolvedTargetId] : [resolvedTargetId, canonical]; - const existing = sqliteService.queryOne<{ id: number }>( - 'SELECT id FROM spouse_edge WHERE person1_id = @p1 AND person2_id = @p2', - { p1, p2 } + 'INSERT OR IGNORE INTO database_membership (db_id, person_id) VALUES (@dbId, @personId)', + { dbId, personId: resolvedTargetId } ); - if (existing) { - return res.status(409).json({ success: false, error: 'This spouse relationship already exists' }); - } - sqliteService.run( - `INSERT INTO spouse_edge (person1_id, person2_id, source, confidence) - VALUES (@p1, @p2, 'manual', 1.0)`, - { p1, p2 } - ); - } else if (relationshipType === 'child') { - // The current person is the parent, target is the child - const existing = sqliteService.queryOne<{ id: number }>( - 'SELECT id FROM parent_edge WHERE child_id = @childId AND parent_id = @parentId', - { childId: resolvedTargetId, parentId: canonical } - ); - if (existing) { - return res.status(409).json({ success: false, error: 'This child relationship already exists' }); + if (relationshipType === 'father' || relationshipType === 'mother') { + sqliteService.run( + `INSERT INTO parent_edge (child_id, parent_id, parent_role, source, confidence) + VALUES (@childId, @parentId, @role, 'manual', 1.0)`, + { childId: canonical, parentId: resolvedTargetId, role: relationshipType } + ); + } else if (relationshipType === 'spouse') { + // Normalize ordering (smaller ID first) to prevent duplicate pairs + const [p1, p2] = canonical < resolvedTargetId ? [canonical, resolvedTargetId] : [resolvedTargetId, canonical]; + sqliteService.run( + `INSERT INTO spouse_edge (person1_id, person2_id, source, confidence) + VALUES (@p1, @p2, 'manual', 1.0)`, + { p1, p2 } + ); + } else if (relationshipType === 'child') { + sqliteService.run( + `INSERT INTO parent_edge (child_id, parent_id, parent_role, source, confidence) + VALUES (@childId, @parentId, @role, 'manual', 1.0)`, + { childId: resolvedTargetId, parentId: canonical, role: childParentRole } + ); } + }); - // Determine parent role from current person's gender - const row = sqliteService.queryOne<{ gender: string }>( - 'SELECT gender FROM person WHERE person_id = @id', - { id: canonical } - ); - const parentRole = row?.gender === 'female' ? 'mother' : row?.gender === 'male' ? 'father' : 'parent'; - - sqliteService.run( - `INSERT INTO parent_edge (child_id, parent_id, parent_role, source, confidence) - VALUES (@childId, @parentId, @role, 'manual', 1.0)`, - { childId: resolvedTargetId, parentId: canonical, role: parentRole } - ); + if (createdNew) { + logger.done('link-relationship', `Created person stub: ${newPerson.name} (${resolvedTargetId})`); } - logger.done('link-relationship', `Linked ${relationshipType}: ${canonical} ↔ ${resolvedTargetId}`); res.json({ @@ -819,17 +848,55 @@ personRoutes.post('/:dbId/:personId/link-relationship', async (req, res, next) = }); }); +/** + * Pre-check whether a relationship edge already exists between two persons. + * Returns an error message if a duplicate exists, otherwise null. + */ +function checkDuplicateEdge( + canonicalId: string, + targetId: string, + relationshipType: string +): string | null { + if (relationshipType === 'father' || relationshipType === 'mother') { + const existing = sqliteService.queryOne<{ id: number }>( + 'SELECT id FROM parent_edge WHERE child_id = @childId AND parent_id = @parentId', + { childId: canonicalId, parentId: targetId } + ); + return existing ? 'This parent relationship already exists' : null; + } + if (relationshipType === 'spouse') { + const [p1, p2] = canonicalId < targetId ? [canonicalId, targetId] : [targetId, canonicalId]; + const existing = sqliteService.queryOne<{ id: number }>( + 'SELECT id FROM spouse_edge WHERE person1_id = @p1 AND person2_id = @p2', + { p1, p2 } + ); + return existing ? 'This spouse relationship already exists' : null; + } + if (relationshipType === 'child') { + const existing = sqliteService.queryOne<{ id: number }>( + 'SELECT id FROM parent_edge WHERE child_id = @childId AND parent_id = @parentId', + { childId: targetId, parentId: canonicalId } + ); + return existing ? 'This child relationship already exists' : null; + } + return null; +} + // DELETE /api/persons/:dbId/:personId/unlink-relationship // Remove a relationship between two people // Body: { relationshipType: 'father'|'mother'|'spouse'|'child', targetId: string } personRoutes.delete('/:dbId/:personId/unlink-relationship', async (req, res, next) => { - const { personId } = req.params; + const { dbId, personId } = req.params; const { relationshipType, targetId } = req.body; if (!relationshipType || !VALID_RELATIONSHIP_TYPES.includes(relationshipType) || !targetId) { return res.status(400).json({ success: false, error: 'relationshipType and targetId are required' }); } + if (!isCanonicalId(targetId)) { + return res.status(400).json({ success: false, error: 'Invalid targetId format' }); + } + const canonical = resolveCanonicalOrFail(personId, res); if (!canonical) return; @@ -837,6 +904,16 @@ personRoutes.delete('/:dbId/:personId/unlink-relationship', async (req, res, nex return res.status(400).json({ success: false, error: 'SQLite must be enabled' }); } + // Verify both persons belong to this database before modifying edges. + // The membership pre-checks below are sufficient to scope deletes — no + // redundant EXISTS guards needed in the DELETE statements themselves. + if (!isPersonInDatabase(canonical, dbId)) { + return res.status(403).json({ success: false, error: 'Person does not belong to the specified database' }); + } + if (!isPersonInDatabase(targetId, dbId)) { + return res.status(403).json({ success: false, error: 'Target person does not belong to the specified database' }); + } + let deleted = false; if (relationshipType === 'father' || relationshipType === 'mother') { @@ -847,7 +924,8 @@ personRoutes.delete('/:dbId/:personId/unlink-relationship', async (req, res, nex deleted = result.changes > 0; } else if (relationshipType === 'spouse') { const result = sqliteService.run( - 'DELETE FROM spouse_edge WHERE (person1_id = @a AND person2_id = @b) OR (person1_id = @b AND person2_id = @a)', + `DELETE FROM spouse_edge + WHERE (person1_id = @a AND person2_id = @b) OR (person1_id = @b AND person2_id = @a)`, { a: canonical, b: targetId } ); deleted = result.changes > 0; diff --git a/server/src/services/id-mapping.service.ts b/server/src/services/id-mapping.service.ts index 25f610a4..b7e56205 100644 --- a/server/src/services/id-mapping.service.ts +++ b/server/src/services/id-mapping.service.ts @@ -177,21 +177,23 @@ function createPersonStub( ): string { const personId = ulid(); - sqliteService.run( - `INSERT INTO person (person_id, display_name, birth_name, gender, living, bio) - VALUES (@personId, @displayName, @birthName, @gender, @living, @bio)`, - { - personId, - displayName, - birthName: options?.birthName ?? null, - gender: options?.gender ?? 'unknown', - living: options?.living ? 1 : 0, - bio: options?.bio ?? null, - } - ); + sqliteService.transaction(() => { + sqliteService.run( + `INSERT INTO person (person_id, display_name, birth_name, gender, living, bio) + VALUES (@personId, @displayName, @birthName, @gender, @living, @bio)`, + { + personId, + displayName, + birthName: options?.birthName ?? null, + gender: options?.gender ?? 'unknown', + living: options?.living ? 1 : 0, + bio: options?.bio ?? null, + } + ); - // Update FTS index - sqliteService.updatePersonFts(personId, displayName, options?.birthName); + // Update FTS index inside the same transaction so stub is never partially visible + sqliteService.updatePersonFts(personId, displayName, options?.birthName); + }); return personId; } From 97963c919c17b911efb36491fd91bca53665c3bc Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Mon, 6 Apr 2026 21:38:23 -0700 Subject: [PATCH 04/14] fix(relationships): address round 2 review feedback - person.routes: resolve route :dbId to internal db_id via resolveDbId() in quick-search, link-relationship, and unlink-relationship handlers, matching the pattern used by integrity/auditor/search services. Without this, callers passing a legacy/FamilySearch ID silently fail membership checks and create orphan database_membership rows. - quick-search: add ORDER BY display_name, person_id for deterministic autocomplete results - PersonDetail: import RelationshipType from types/relationship instead of from RelationshipModal (reduces coupling) - RelationshipModal: drop now-unused re-export of RelationshipType --- client/src/components/person/PersonDetail.tsx | 2 +- .../components/person/RelationshipModal.tsx | 2 - server/src/routes/person.routes.ts | 39 +++++++++++++------ 3 files changed, 29 insertions(+), 14 deletions(-) diff --git a/client/src/components/person/PersonDetail.tsx b/client/src/components/person/PersonDetail.tsx index c0efc007..964b4bb0 100644 --- a/client/src/components/person/PersonDetail.tsx +++ b/client/src/components/person/PersonDetail.tsx @@ -15,7 +15,7 @@ import { UploadToAncestryDialog } from './UploadToAncestryDialog'; import { ProviderDataTable } from './ProviderDataTable'; import { LinkPlatformDialog } from './LinkPlatformDialog'; import { RelationshipModal } from './RelationshipModal'; -import type { RelationshipType } from './RelationshipModal'; +import type { RelationshipType } from '../../types/relationship'; import { PersonAuditIssues } from './PersonAuditIssues'; diff --git a/client/src/components/person/RelationshipModal.tsx b/client/src/components/person/RelationshipModal.tsx index bb769fde..7716cf6b 100644 --- a/client/src/components/person/RelationshipModal.tsx +++ b/client/src/components/person/RelationshipModal.tsx @@ -4,8 +4,6 @@ import toast from 'react-hot-toast'; import { api } from '../../services/api'; import type { RelationshipType } from '../../types/relationship'; -export type { RelationshipType }; - interface RelationshipModalProps { open: boolean; dbId: string; diff --git a/server/src/routes/person.routes.ts b/server/src/routes/person.routes.ts index fcf7601a..418a2c19 100644 --- a/server/src/routes/person.routes.ts +++ b/server/src/routes/person.routes.ts @@ -9,7 +9,7 @@ import { localOverrideService } from '../services/local-override.service.js'; import { familySearchRefreshService } from '../services/familysearch-refresh.service.js'; import { augmentationService } from '../services/augmentation.service.js'; import { sqliteService } from '../db/sqlite.service.js'; -import { databaseService } from '../services/database.service.js'; +import { databaseService, resolveDbId } from '../services/database.service.js'; import { logger } from '../lib/logger.js'; import type { BuiltInProvider } from '@fsf/shared'; import { PHOTOS_DIR, PROVIDER_CACHE_DIR } from '../utils/paths.js'; @@ -40,7 +40,9 @@ personRoutes.get('/:dbId/quick-search', async (req, res, next) => { return res.json({ success: true, data: [] }); } - const { dbId } = req.params; + const internalDbId = resolveDbId(req.params.dbId); + if (!internalDbId) return res.json({ success: true, data: [] }); + const sanitized = sanitizeFtsQuery(q); if (!sanitized) return res.json({ success: true, data: [] }); const ftsQuery = `"${sanitized}"*`; @@ -63,8 +65,9 @@ personRoutes.get('/:dbId/quick-search', async (req, res, next) => { ) ve ON ve.person_id = p.person_id WHERE dm.db_id = @dbId AND p.person_id IN (SELECT person_id FROM person_fts WHERE person_fts MATCH @q) + ORDER BY p.display_name, p.person_id LIMIT 20`, - { dbId, q: ftsQuery } + { dbId: internalDbId, q: ftsQuery } ); const data = results.map(r => ({ @@ -722,7 +725,7 @@ function isPersonInDatabase(personId: string, dbId: string): boolean { // Link an existing person or create a new stub as parent/spouse/child // Body: { relationshipType: 'father'|'mother'|'spouse'|'child', targetId?: string, newPerson?: { name: string, gender?: string } } personRoutes.post('/:dbId/:personId/link-relationship', async (req, res, next) => { - const { dbId, personId } = req.params; + const { personId } = req.params; const { relationshipType, targetId, newPerson } = req.body; if (!relationshipType || !VALID_RELATIONSHIP_TYPES.includes(relationshipType)) { @@ -733,13 +736,21 @@ personRoutes.post('/:dbId/:personId/link-relationship', async (req, res, next) = return res.status(400).json({ success: false, error: 'Provide either targetId (existing person) or newPerson.name (to create a stub)' }); } - const canonical = resolveCanonicalOrFail(personId, res); - if (!canonical) return; - if (!databaseService.isSqliteEnabled()) { return res.status(400).json({ success: false, error: 'SQLite must be enabled for relationship linking' }); } + // Resolve route :dbId (which may be a legacy/FS ID) to the internal db_id + // used by database_membership. Without this, callers passing a non-internal + // identifier silently fail membership checks and create orphan rows. + const dbId = resolveDbId(req.params.dbId); + if (!dbId) { + return res.status(404).json({ success: false, error: 'Database not found' }); + } + + const canonical = resolveCanonicalOrFail(personId, res); + if (!canonical) return; + // Verify the source person belongs to this database if (!isPersonInDatabase(canonical, dbId)) { return res.status(403).json({ success: false, error: 'Person does not belong to the specified database' }); @@ -886,7 +897,7 @@ function checkDuplicateEdge( // Remove a relationship between two people // Body: { relationshipType: 'father'|'mother'|'spouse'|'child', targetId: string } personRoutes.delete('/:dbId/:personId/unlink-relationship', async (req, res, next) => { - const { dbId, personId } = req.params; + const { personId } = req.params; const { relationshipType, targetId } = req.body; if (!relationshipType || !VALID_RELATIONSHIP_TYPES.includes(relationshipType) || !targetId) { @@ -897,13 +908,19 @@ personRoutes.delete('/:dbId/:personId/unlink-relationship', async (req, res, nex return res.status(400).json({ success: false, error: 'Invalid targetId format' }); } - const canonical = resolveCanonicalOrFail(personId, res); - if (!canonical) return; - if (!databaseService.isSqliteEnabled()) { return res.status(400).json({ success: false, error: 'SQLite must be enabled' }); } + // Resolve route :dbId (which may be a legacy/FS ID) to internal db_id + const dbId = resolveDbId(req.params.dbId); + if (!dbId) { + return res.status(404).json({ success: false, error: 'Database not found' }); + } + + const canonical = resolveCanonicalOrFail(personId, res); + if (!canonical) return; + // Verify both persons belong to this database before modifying edges. // The membership pre-checks below are sufficient to scope deletes — no // redundant EXISTS guards needed in the DELETE statements themselves. From 47fe1cb8bd6307f1df0807d62bb4c022c175fc25 Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Mon, 6 Apr 2026 21:48:05 -0700 Subject: [PATCH 05/14] fix(relationships): address round 3 review feedback - person.routes link-relationship: use INSERT OR IGNORE for edge inserts to defend against races between pre-check and write; check changes count and return 409 if no row was inserted (duplicate snuck in) - person.routes link-relationship: refresh database_info.person_count cache when a new membership row is added so the database listing reflects new persons immediately - api.ts: tighten linkRelationship/unlinkRelationship response types from string to RelationshipType for full type-safety end to end --- client/src/services/api.ts | 4 ++-- server/src/routes/person.routes.ts | 34 ++++++++++++++++++++++++------ 2 files changed, 30 insertions(+), 8 deletions(-) diff --git a/client/src/services/api.ts b/client/src/services/api.ts index fcaaefc8..25b670b5 100644 --- a/client/src/services/api.ts +++ b/client/src/services/api.ts @@ -719,7 +719,7 @@ export const api = { fetchJson<{ personId: string; targetId: string; - relationshipType: string; + relationshipType: RelationshipType; createdNew: boolean; }>( `/persons/${dbId}/${personId}/link-relationship`, @@ -730,7 +730,7 @@ export const api = { ), unlinkRelationship: (dbId: string, personId: string, relationshipType: RelationshipType, targetId: string) => - fetchJson<{ personId: string; targetId: string; relationshipType: string }>( + fetchJson<{ personId: string; targetId: string; relationshipType: RelationshipType }>( `/persons/${dbId}/${personId}/unlink-relationship`, { method: 'DELETE', diff --git a/server/src/routes/person.routes.ts b/server/src/routes/person.routes.ts index 418a2c19..1ab5028f 100644 --- a/server/src/routes/person.routes.ts +++ b/server/src/routes/person.routes.ts @@ -810,6 +810,10 @@ personRoutes.post('/:dbId/:personId/link-relationship', async (req, res, next) = // Single transaction wraps stub creation + membership + edge insertion so a // failure anywhere rolls back all writes (no orphaned stubs or memberships). + // Edge inserts use INSERT OR IGNORE to defend against races between the + // pre-check and the write — `edgeInserted` reflects whether the edge was + // actually new and is checked outside the transaction to map to a 409. + let edgeInserted = false; sqliteService.transaction(() => { if (createdNew) { resolvedTargetId = idMappingService.createPersonStub(newPerson.name, { gender: stubGender }); @@ -820,29 +824,47 @@ personRoutes.post('/:dbId/:personId/link-relationship', async (req, res, next) = { dbId, personId: resolvedTargetId } ); + let edgeResult: { changes: number } | undefined; if (relationshipType === 'father' || relationshipType === 'mother') { - sqliteService.run( - `INSERT INTO parent_edge (child_id, parent_id, parent_role, source, confidence) + edgeResult = sqliteService.run( + `INSERT OR IGNORE INTO parent_edge (child_id, parent_id, parent_role, source, confidence) VALUES (@childId, @parentId, @role, 'manual', 1.0)`, { childId: canonical, parentId: resolvedTargetId, role: relationshipType } ); } else if (relationshipType === 'spouse') { // Normalize ordering (smaller ID first) to prevent duplicate pairs const [p1, p2] = canonical < resolvedTargetId ? [canonical, resolvedTargetId] : [resolvedTargetId, canonical]; - sqliteService.run( - `INSERT INTO spouse_edge (person1_id, person2_id, source, confidence) + edgeResult = sqliteService.run( + `INSERT OR IGNORE INTO spouse_edge (person1_id, person2_id, source, confidence) VALUES (@p1, @p2, 'manual', 1.0)`, { p1, p2 } ); } else if (relationshipType === 'child') { - sqliteService.run( - `INSERT INTO parent_edge (child_id, parent_id, parent_role, source, confidence) + edgeResult = sqliteService.run( + `INSERT OR IGNORE INTO parent_edge (child_id, parent_id, parent_role, source, confidence) VALUES (@childId, @parentId, @role, 'manual', 1.0)`, { childId: resolvedTargetId, parentId: canonical, role: childParentRole } ); } + edgeInserted = (edgeResult?.changes ?? 0) > 0; + + // Refresh cached person_count so the database listing reflects the new + // membership row immediately. Other tables update via triggers, but + // database_info.person_count is a denormalized cache. + if (edgeInserted) { + sqliteService.run( + `UPDATE database_info + SET person_count = (SELECT COUNT(*) FROM database_membership WHERE db_id = @dbId) + WHERE db_id = @dbId`, + { dbId } + ); + } }); + if (!edgeInserted) { + return res.status(409).json({ success: false, error: 'This relationship already exists' }); + } + if (createdNew) { logger.done('link-relationship', `Created person stub: ${newPerson.name} (${resolvedTargetId})`); } From 43594802148a0bce98b6ecf01b1bda408997387f Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Mon, 6 Apr 2026 22:02:11 -0700 Subject: [PATCH 06/14] fix(relationships): address round 4 review feedback + add tests - person.routes link-relationship: trim newPerson.name and reject blank or whitespace-only values to prevent stub persons with empty display names - RelationshipModal: type onLinked as () => void | Promise and await it before closing the modal, so the modal stays open visually until the parent's reload completes - tests: add tests/integration/api/relationships.spec.ts with 17 cases covering quick-search (db scoping, query length, no-match), link (validation, self-link, cross-db, stub creation, duplicate, parent), and unlink (delete, 404, cross-db rejection) - tests/integration/setup: register inline simplified versions of quick-search, link-relationship, and unlink-relationship endpoints matching production validation, scoping, and idempotent insert behavior --- .../components/person/RelationshipModal.tsx | 6 +- server/src/routes/person.routes.ts | 9 +- tests/integration/api/relationships.spec.ts | 230 ++++++++++++++++++ tests/integration/setup.ts | 146 +++++++++++ 4 files changed, 385 insertions(+), 6 deletions(-) create mode 100644 tests/integration/api/relationships.spec.ts diff --git a/client/src/components/person/RelationshipModal.tsx b/client/src/components/person/RelationshipModal.tsx index 7716cf6b..e16cfbbc 100644 --- a/client/src/components/person/RelationshipModal.tsx +++ b/client/src/components/person/RelationshipModal.tsx @@ -10,7 +10,7 @@ interface RelationshipModalProps { personId: string; initialType?: RelationshipType; onClose: () => void; - onLinked: () => void; + onLinked: () => void | Promise; } interface QuickSearchResult { @@ -99,7 +99,7 @@ export function RelationshipModal({ open, dbId, personId, initialType, onClose, }); setLinkingId(null); if (!result) return; - onLinked(); + await onLinked(); onClose(); }; @@ -113,7 +113,7 @@ export function RelationshipModal({ open, dbId, personId, initialType, onClose, }); setLinkingId(null); if (!result) return; - onLinked(); + await onLinked(); onClose(); }; diff --git a/server/src/routes/person.routes.ts b/server/src/routes/person.routes.ts index 1ab5028f..6638e89d 100644 --- a/server/src/routes/person.routes.ts +++ b/server/src/routes/person.routes.ts @@ -732,7 +732,10 @@ personRoutes.post('/:dbId/:personId/link-relationship', async (req, res, next) = return res.status(400).json({ success: false, error: `Invalid relationshipType. Must be one of: ${VALID_RELATIONSHIP_TYPES.join(', ')}` }); } - if (!targetId && !newPerson?.name) { + // Normalize new-person name once and reject blank/whitespace-only values + const trimmedNewPersonName = + typeof newPerson?.name === 'string' ? newPerson.name.trim() : ''; + if (!targetId && !trimmedNewPersonName) { return res.status(400).json({ success: false, error: 'Provide either targetId (existing person) or newPerson.name (to create a stub)' }); } @@ -816,7 +819,7 @@ personRoutes.post('/:dbId/:personId/link-relationship', async (req, res, next) = let edgeInserted = false; sqliteService.transaction(() => { if (createdNew) { - resolvedTargetId = idMappingService.createPersonStub(newPerson.name, { gender: stubGender }); + resolvedTargetId = idMappingService.createPersonStub(trimmedNewPersonName, { gender: stubGender }); } sqliteService.run( @@ -866,7 +869,7 @@ personRoutes.post('/:dbId/:personId/link-relationship', async (req, res, next) = } if (createdNew) { - logger.done('link-relationship', `Created person stub: ${newPerson.name} (${resolvedTargetId})`); + logger.done('link-relationship', `Created person stub: ${trimmedNewPersonName} (${resolvedTargetId})`); } logger.done('link-relationship', `Linked ${relationshipType}: ${canonical} ↔ ${resolvedTargetId}`); diff --git a/tests/integration/api/relationships.spec.ts b/tests/integration/api/relationships.spec.ts new file mode 100644 index 00000000..7924957e --- /dev/null +++ b/tests/integration/api/relationships.spec.ts @@ -0,0 +1,230 @@ +/** + * Relationship link/unlink + quick-search API tests + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import request from 'supertest'; +import { createTestApp, seedTestData, type TestContext } from '../setup'; + +describe('Relationship Routes', () => { + let ctx: TestContext; + + // Use beforeEach so each test starts with a fresh DB — link/unlink mutate state + beforeEach(() => { + ctx = createTestApp(); + seedTestData(ctx.db); + }); + + afterEach(() => { + ctx.close(); + }); + + describe('GET /api/persons/:dbId/quick-search', () => { + it('returns matching persons scoped to the database', async () => { + const response = await request(ctx.app) + .get('/api/persons/test-db/quick-search?q=John') + .expect(200); + + expect(response.body.success).toBe(true); + expect(response.body.data).toHaveLength(1); + expect(response.body.data[0].personId).toBe('PERSON-001'); + expect(response.body.data[0].displayName).toBe('John Smith'); + }); + + it('returns empty for queries shorter than 2 chars', async () => { + const response = await request(ctx.app) + .get('/api/persons/test-db/quick-search?q=J') + .expect(200); + + expect(response.body.data).toEqual([]); + }); + + it('returns empty when no matches', async () => { + const response = await request(ctx.app) + .get('/api/persons/test-db/quick-search?q=Zelda') + .expect(200); + + expect(response.body.data).toEqual([]); + }); + + it('does not return persons from another database', async () => { + // Create a second database with a person who matches the same query + ctx.db.prepare(` + INSERT INTO person (person_id, display_name, gender, living) + VALUES ('PERSON-OTHER', 'John Otherson', 'male', 0) + `).run(); + ctx.db.prepare(` + INSERT INTO database_info (db_id, root_id, root_name, source_provider) + VALUES ('other-db', 'PERSON-OTHER', 'John Otherson', 'test') + `).run(); + ctx.db.prepare(` + INSERT INTO database_membership (db_id, person_id) VALUES ('other-db', 'PERSON-OTHER') + `).run(); + + const response = await request(ctx.app) + .get('/api/persons/test-db/quick-search?q=John') + .expect(200); + + const ids = response.body.data.map((r: { personId: string }) => r.personId); + expect(ids).toContain('PERSON-001'); + expect(ids).not.toContain('PERSON-OTHER'); + }); + }); + + describe('POST /api/persons/:dbId/:personId/link-relationship', () => { + it('rejects invalid relationshipType', async () => { + const response = await request(ctx.app) + .post('/api/persons/test-db/PERSON-001/link-relationship') + .send({ relationshipType: 'cousin', targetId: 'PERSON-002' }) + .expect(400); + + expect(response.body.error).toContain('Invalid relationshipType'); + }); + + it('rejects missing targetId AND newPerson.name', async () => { + const response = await request(ctx.app) + .post('/api/persons/test-db/PERSON-001/link-relationship') + .send({ relationshipType: 'spouse' }) + .expect(400); + + expect(response.body.error).toContain('targetId'); + }); + + it('rejects whitespace-only newPerson.name', async () => { + const response = await request(ctx.app) + .post('/api/persons/test-db/PERSON-001/link-relationship') + .send({ relationshipType: 'spouse', newPerson: { name: ' ' } }) + .expect(400); + }); + + it('rejects self-link', async () => { + const response = await request(ctx.app) + .post('/api/persons/test-db/PERSON-001/link-relationship') + .send({ relationshipType: 'spouse', targetId: 'PERSON-001' }) + .expect(400); + + expect(response.body.error).toContain('themselves'); + }); + + it('rejects when source person is not in this database', async () => { + // Create an isolated person not in test-db + ctx.db.prepare(`INSERT INTO person (person_id, display_name, gender, living) VALUES ('ORPHAN', 'Orphan', 'unknown', 0)`).run(); + + const response = await request(ctx.app) + .post('/api/persons/test-db/ORPHAN/link-relationship') + .send({ relationshipType: 'spouse', targetId: 'PERSON-002' }) + .expect(403); + }); + + it('rejects 404 when targetId does not exist', async () => { + const response = await request(ctx.app) + .post('/api/persons/test-db/PERSON-001/link-relationship') + .send({ relationshipType: 'spouse', targetId: 'NONEXISTENT' }) + .expect(404); + }); + + it('creates a spouse edge between existing persons', async () => { + // Add a candidate spouse to the database + ctx.db.prepare(`INSERT INTO person (person_id, display_name, gender, living) VALUES ('SPOUSE-1', 'Jane Doe', 'female', 0)`).run(); + ctx.db.prepare(`INSERT INTO database_membership (db_id, person_id) VALUES ('test-db', 'SPOUSE-1')`).run(); + + const response = await request(ctx.app) + .post('/api/persons/test-db/PERSON-001/link-relationship') + .send({ relationshipType: 'spouse', targetId: 'SPOUSE-1' }) + .expect(200); + + expect(response.body.data.relationshipType).toBe('spouse'); + + const edge = ctx.db.prepare(` + SELECT * FROM spouse_edge + WHERE (person1_id = 'PERSON-001' AND person2_id = 'SPOUSE-1') + OR (person1_id = 'SPOUSE-1' AND person2_id = 'PERSON-001') + `).get(); + expect(edge).toBeDefined(); + }); + + it('creates a stub person and links as parent', async () => { + const response = await request(ctx.app) + .post('/api/persons/test-db/PERSON-001/link-relationship') + .send({ + relationshipType: 'father', + newPerson: { name: 'Stub Father' } + }); + + // PERSON-001 already has a father (PERSON-002) in seed data — linking + // a NEW father should still work since the constraint is on + // (child_id, parent_id) not on parent_role. + expect(response.status).toBe(200); + expect(response.body.data.createdNew).toBe(true); + + const stubId = response.body.data.targetId; + const stub = ctx.db.prepare('SELECT * FROM person WHERE person_id = ?').get(stubId) as { display_name: string; gender: string }; + expect(stub.display_name).toBe('Stub Father'); + expect(stub.gender).toBe('male'); // coerced from relationshipType + + // Stub should be a member of the same database + const membership = ctx.db.prepare( + 'SELECT 1 FROM database_membership WHERE db_id = ? AND person_id = ?' + ).get('test-db', stubId); + expect(membership).toBeDefined(); + }); + + it('rejects duplicate parent edge', async () => { + // PERSON-001 already has father PERSON-002 from seed data + const response = await request(ctx.app) + .post('/api/persons/test-db/PERSON-001/link-relationship') + .send({ relationshipType: 'father', targetId: 'PERSON-002' }) + .expect(409); + + expect(response.body.error).toMatch(/already exists/i); + }); + }); + + describe('DELETE /api/persons/:dbId/:personId/unlink-relationship', () => { + it('removes a parent edge', async () => { + const response = await request(ctx.app) + .delete('/api/persons/test-db/PERSON-001/unlink-relationship') + .send({ relationshipType: 'father', targetId: 'PERSON-002' }) + .expect(200); + + const edge = ctx.db.prepare( + 'SELECT 1 FROM parent_edge WHERE child_id = ? AND parent_id = ?' + ).get('PERSON-001', 'PERSON-002'); + expect(edge).toBeUndefined(); + }); + + it('returns 404 when no matching edge exists', async () => { + // Add an unrelated person to test-db so the membership check passes + ctx.db.prepare(`INSERT INTO person (person_id, display_name, gender, living) VALUES ('UNRELATED', 'Unrelated', 'unknown', 0)`).run(); + ctx.db.prepare(`INSERT INTO database_membership (db_id, person_id) VALUES ('test-db', 'UNRELATED')`).run(); + + const response = await request(ctx.app) + .delete('/api/persons/test-db/PERSON-001/unlink-relationship') + .send({ relationshipType: 'spouse', targetId: 'UNRELATED' }) + .expect(404); + }); + + it('rejects unlink when source person is not in this database', async () => { + ctx.db.prepare(`INSERT INTO person (person_id, display_name, gender, living) VALUES ('ORPHAN', 'Orphan', 'unknown', 0)`).run(); + + const response = await request(ctx.app) + .delete('/api/persons/test-db/ORPHAN/unlink-relationship') + .send({ relationshipType: 'father', targetId: 'PERSON-002' }) + .expect(403); + }); + + it('rejects unlink when target is in a different database', async () => { + ctx.db.prepare(`INSERT INTO person (person_id, display_name, gender, living) VALUES ('OTHER-PERSON', 'Other', 'unknown', 0)`).run(); + ctx.db.prepare(` + INSERT INTO database_info (db_id, root_id, root_name, source_provider) + VALUES ('other-db', 'OTHER-PERSON', 'Other', 'test') + `).run(); + ctx.db.prepare(`INSERT INTO database_membership (db_id, person_id) VALUES ('other-db', 'OTHER-PERSON')`).run(); + + const response = await request(ctx.app) + .delete('/api/persons/test-db/PERSON-001/unlink-relationship') + .send({ relationshipType: 'spouse', targetId: 'OTHER-PERSON' }) + .expect(403); + }); + }); +}); diff --git a/tests/integration/setup.ts b/tests/integration/setup.ts index ea9a5798..843f48ae 100644 --- a/tests/integration/setup.ts +++ b/tests/integration/setup.ts @@ -110,6 +110,27 @@ export const createTestApp = (): TestContext => { }); }); + // GET /api/persons/:dbId/quick-search - FTS-style autocomplete + // Must be registered before /:dbId/:personId to avoid route conflict + app.get('/api/persons/:dbId/quick-search', (req, res) => { + const q = ((req.query.q as string) || '').trim(); + if (!q || q.length < 2) { + return res.json({ success: true, data: [] }); + } + + // Simplified search: substring match scoped by database_membership + const results = db.prepare(` + SELECT p.person_id as personId, p.display_name as displayName, p.gender, p.birth_name as birthName + FROM person p + JOIN database_membership dm ON p.person_id = dm.person_id + WHERE dm.db_id = ? AND p.display_name LIKE ? + ORDER BY p.display_name, p.person_id + LIMIT 20 + `).all(req.params.dbId, `%${q}%`); + + res.json({ success: true, data: results }); + }); + // GET /api/persons/:dbId/:personId - Get single person app.get('/api/persons/:dbId/:personId', (req, res) => { const person = db.prepare(` @@ -126,6 +147,131 @@ export const createTestApp = (): TestContext => { res.json({ success: true, data: person }); }); + // POST /api/persons/:dbId/:personId/link-relationship + // Mirrors production validation, scoping, and idempotent insert behavior + const VALID_REL_TYPES = ['father', 'mother', 'spouse', 'child']; + const isInDb = (personId: string, dbId: string): boolean => + !!db.prepare('SELECT 1 FROM database_membership WHERE db_id = ? AND person_id = ?') + .get(dbId, personId); + + app.post('/api/persons/:dbId/:personId/link-relationship', (req, res) => { + const { dbId, personId } = req.params; + const { relationshipType, targetId, newPerson } = req.body; + + if (!relationshipType || !VALID_REL_TYPES.includes(relationshipType)) { + return res.status(400).json({ success: false, error: 'Invalid relationshipType' }); + } + const trimmedName = typeof newPerson?.name === 'string' ? newPerson.name.trim() : ''; + if (!targetId && !trimmedName) { + return res.status(400).json({ success: false, error: 'Provide either targetId or newPerson.name' }); + } + if (!isInDb(personId, dbId)) { + return res.status(403).json({ success: false, error: 'Person does not belong to the specified database' }); + } + + let resolvedTargetId: string; + let createdNew = false; + + if (targetId) { + if (targetId === personId) { + return res.status(400).json({ success: false, error: 'Cannot link a person to themselves' }); + } + const exists = db.prepare('SELECT 1 FROM person WHERE person_id = ?').get(targetId); + if (!exists) { + return res.status(404).json({ success: false, error: 'Target person not found' }); + } + resolvedTargetId = targetId; + } else { + const requestedGender = typeof newPerson?.gender === 'string' ? newPerson.gender.toLowerCase() : ''; + const stubGender = + ['male', 'female', 'unknown'].includes(requestedGender) + ? requestedGender + : relationshipType === 'father' ? 'male' : relationshipType === 'mother' ? 'female' : 'unknown'; + // Generate a simple unique stub id for tests + resolvedTargetId = `STUB-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; + db.prepare(`INSERT INTO person (person_id, display_name, gender, living) VALUES (?, ?, ?, 0)`) + .run(resolvedTargetId, trimmedName, stubGender); + createdNew = true; + } + + // Idempotent membership + edge insert in a transaction + let edgeInserted = false; + db.transaction(() => { + db.prepare('INSERT OR IGNORE INTO database_membership (db_id, person_id) VALUES (?, ?)') + .run(dbId, resolvedTargetId); + + let result; + if (relationshipType === 'father' || relationshipType === 'mother') { + result = db.prepare(` + INSERT OR IGNORE INTO parent_edge (child_id, parent_id, parent_role, source, confidence) + VALUES (?, ?, ?, 'manual', 1.0) + `).run(personId, resolvedTargetId, relationshipType); + } else if (relationshipType === 'spouse') { + const [p1, p2] = personId < resolvedTargetId ? [personId, resolvedTargetId] : [resolvedTargetId, personId]; + result = db.prepare(` + INSERT OR IGNORE INTO spouse_edge (person1_id, person2_id, source, confidence) + VALUES (?, ?, 'manual', 1.0) + `).run(p1, p2); + } else { + // child + result = db.prepare(` + INSERT OR IGNORE INTO parent_edge (child_id, parent_id, parent_role, source, confidence) + VALUES (?, ?, 'parent', 'manual', 1.0) + `).run(resolvedTargetId, personId); + } + edgeInserted = (result?.changes ?? 0) > 0; + })(); + + if (!edgeInserted) { + return res.status(409).json({ success: false, error: 'This relationship already exists' }); + } + + res.json({ + success: true, + data: { personId, targetId: resolvedTargetId, relationshipType, createdNew } + }); + }); + + // DELETE /api/persons/:dbId/:personId/unlink-relationship + app.delete('/api/persons/:dbId/:personId/unlink-relationship', (req, res) => { + const { dbId, personId } = req.params; + const { relationshipType, targetId } = req.body; + + if (!relationshipType || !VALID_REL_TYPES.includes(relationshipType) || !targetId) { + return res.status(400).json({ success: false, error: 'relationshipType and targetId are required' }); + } + if (!isInDb(personId, dbId)) { + return res.status(403).json({ success: false, error: 'Person does not belong to the specified database' }); + } + if (!isInDb(targetId, dbId)) { + return res.status(403).json({ success: false, error: 'Target person does not belong to the specified database' }); + } + + let deleted = false; + if (relationshipType === 'father' || relationshipType === 'mother') { + const result = db.prepare('DELETE FROM parent_edge WHERE child_id = ? AND parent_id = ?') + .run(personId, targetId); + deleted = result.changes > 0; + } else if (relationshipType === 'spouse') { + const result = db.prepare(` + DELETE FROM spouse_edge + WHERE (person1_id = ? AND person2_id = ?) OR (person1_id = ? AND person2_id = ?) + `).run(personId, targetId, targetId, personId); + deleted = result.changes > 0; + } else { + // child + const result = db.prepare('DELETE FROM parent_edge WHERE child_id = ? AND parent_id = ?') + .run(targetId, personId); + deleted = result.changes > 0; + } + + if (!deleted) { + return res.status(404).json({ success: false, error: 'Relationship not found' }); + } + + res.json({ success: true, data: { personId, targetId, relationshipType } }); + }); + // GET /api/search/:dbId - Search persons app.get('/api/search/:dbId', (req, res) => { const q = (req.query.q as string) || ''; From 954abf0c586aaa40c26277edb3874199e3fc1e8d Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Mon, 6 Apr 2026 22:11:19 -0700 Subject: [PATCH 07/14] fix(relationships): address round 5 review feedback - person.routes link-relationship: insert membership only AFTER confirming edge insert was new. Previously, INSERT OR IGNORE racing with a concurrent edge insert would still write the membership row even when returning 409. Now race-induced 409s never mutate state. - tests/setup link-relationship: same ordering fix; reword 'mirrors production validation' comment to explicitly call out divergences (no canonical-ULID checks, no resolveDbId, simple stub IDs) --- server/src/routes/person.routes.ts | 27 ++++++++++++++------------- tests/integration/setup.ts | 16 +++++++++++----- 2 files changed, 25 insertions(+), 18 deletions(-) diff --git a/server/src/routes/person.routes.ts b/server/src/routes/person.routes.ts index 6638e89d..225455ec 100644 --- a/server/src/routes/person.routes.ts +++ b/server/src/routes/person.routes.ts @@ -811,22 +811,19 @@ personRoutes.post('/:dbId/:personId/link-relationship', async (req, res, next) = childParentRole = row?.gender === 'female' ? 'mother' : row?.gender === 'male' ? 'father' : 'parent'; } - // Single transaction wraps stub creation + membership + edge insertion so a - // failure anywhere rolls back all writes (no orphaned stubs or memberships). - // Edge inserts use INSERT OR IGNORE to defend against races between the - // pre-check and the write — `edgeInserted` reflects whether the edge was - // actually new and is checked outside the transaction to map to a 409. + // Single transaction wraps stub creation + edge insertion + membership so a + // failure anywhere rolls back all writes. Edge inserts use INSERT OR IGNORE + // to defend against a race between the pre-check and the write. The + // membership insert is performed AFTER confirming the edge was actually new, + // so a 409 response from a race never mutates state. Stubs use fresh ULIDs + // so their edge insert can never collide; for the existing-target case the + // pre-check usually catches duplicates and the OR IGNORE handles the rest. let edgeInserted = false; sqliteService.transaction(() => { if (createdNew) { resolvedTargetId = idMappingService.createPersonStub(trimmedNewPersonName, { gender: stubGender }); } - sqliteService.run( - 'INSERT OR IGNORE INTO database_membership (db_id, person_id) VALUES (@dbId, @personId)', - { dbId, personId: resolvedTargetId } - ); - let edgeResult: { changes: number } | undefined; if (relationshipType === 'father' || relationshipType === 'mother') { edgeResult = sqliteService.run( @@ -851,10 +848,14 @@ personRoutes.post('/:dbId/:personId/link-relationship', async (req, res, next) = } edgeInserted = (edgeResult?.changes ?? 0) > 0; - // Refresh cached person_count so the database listing reflects the new - // membership row immediately. Other tables update via triggers, but - // database_info.person_count is a denormalized cache. + // Only mutate membership and the cached person_count when the edge was + // actually new — otherwise an OR-IGNORE no-op (race) would leave behind + // an orphan membership row even though the response is a 409. if (edgeInserted) { + sqliteService.run( + 'INSERT OR IGNORE INTO database_membership (db_id, person_id) VALUES (@dbId, @personId)', + { dbId, personId: resolvedTargetId } + ); sqliteService.run( `UPDATE database_info SET person_count = (SELECT COUNT(*) FROM database_membership WHERE db_id = @dbId) diff --git a/tests/integration/setup.ts b/tests/integration/setup.ts index 843f48ae..4d7190d7 100644 --- a/tests/integration/setup.ts +++ b/tests/integration/setup.ts @@ -148,7 +148,10 @@ export const createTestApp = (): TestContext => { }); // POST /api/persons/:dbId/:personId/link-relationship - // Mirrors production validation, scoping, and idempotent insert behavior + // Simplified version of the production handler for integration testing. + // Intentionally diverges from production: no canonical-ULID format checks, + // no resolveDbId mapping (route :dbId is treated as the literal db_id), + // and stub IDs are short test strings instead of ULIDs. const VALID_REL_TYPES = ['father', 'mother', 'spouse', 'child']; const isInDb = (personId: string, dbId: string): boolean => !!db.prepare('SELECT 1 FROM database_membership WHERE db_id = ? AND person_id = ?') @@ -194,12 +197,10 @@ export const createTestApp = (): TestContext => { createdNew = true; } - // Idempotent membership + edge insert in a transaction + // Edge insert FIRST, then membership only if the edge was new — matches + // production ordering so a race-induced 409 never mutates membership state. let edgeInserted = false; db.transaction(() => { - db.prepare('INSERT OR IGNORE INTO database_membership (db_id, person_id) VALUES (?, ?)') - .run(dbId, resolvedTargetId); - let result; if (relationshipType === 'father' || relationshipType === 'mother') { result = db.prepare(` @@ -220,6 +221,11 @@ export const createTestApp = (): TestContext => { `).run(resolvedTargetId, personId); } edgeInserted = (result?.changes ?? 0) > 0; + + if (edgeInserted) { + db.prepare('INSERT OR IGNORE INTO database_membership (db_id, person_id) VALUES (?, ?)') + .run(dbId, resolvedTargetId); + } })(); if (!edgeInserted) { From 85b07abbcef2775981433d90ecc64bda1a952c7e Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Mon, 6 Apr 2026 22:20:40 -0700 Subject: [PATCH 08/14] fix(relationships): guard doSearch against out-of-order responses Track a monotonically increasing search request id and drop responses whose id no longer matches the latest. Without this, fast typing could let an earlier search resolve after a later one and overwrite results or flip the spinner off prematurely. --- .../src/components/person/RelationshipModal.tsx | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/client/src/components/person/RelationshipModal.tsx b/client/src/components/person/RelationshipModal.tsx index e16cfbbc..91dd4464 100644 --- a/client/src/components/person/RelationshipModal.tsx +++ b/client/src/components/person/RelationshipModal.tsx @@ -39,6 +39,9 @@ export function RelationshipModal({ open, dbId, personId, initialType, onClose, const [newGender, setNewGender] = useState<'male' | 'female' | 'unknown'>('unknown'); const inputRef = useRef(null); const debounceRef = useRef>(); + // Monotonically increasing request id so out-of-order responses from + // earlier searches don't overwrite results from a newer search. + const searchRequestIdRef = useRef(0); useEffect(() => { if (open) { @@ -50,9 +53,12 @@ export function RelationshipModal({ open, dbId, personId, initialType, onClose, setLinkingId(null); setNewName(''); setNewGender('unknown'); + // Invalidate any in-flight requests from a previous open + searchRequestIdRef.current += 1; setTimeout(() => inputRef.current?.focus(), 100); } else { if (debounceRef.current) clearTimeout(debounceRef.current); + searchRequestIdRef.current += 1; } }, [open, initialType]); @@ -73,15 +79,23 @@ export function RelationshipModal({ open, dbId, personId, initialType, onClose, setResults([]); return; } + const requestId = ++searchRequestIdRef.current; setSearching(true); try { const data = await api.quickSearchPersons(dbId, q); + // Drop the response if a newer search has been issued in the meantime + if (requestId !== searchRequestIdRef.current) return; setResults(data.filter(r => r.personId !== personId)); } catch (error) { + if (requestId !== searchRequestIdRef.current) return; console.error('Failed to search persons', error); toast.error('Failed to search persons. Please try again.'); } finally { - setSearching(false); + // Only clear the spinner for the latest request — earlier requests + // resolving late must not flip it off while a newer one is still active + if (requestId === searchRequestIdRef.current) { + setSearching(false); + } } }, [dbId, personId]); From 89a95454b12082f9ad08dadccf14541d29f8f376 Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Mon, 6 Apr 2026 22:27:35 -0700 Subject: [PATCH 09/14] test(relationships): include birthYear in test quick-search response Match production response shape so client/contract regressions on the birthYear field are caught in tests. --- tests/integration/setup.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/tests/integration/setup.ts b/tests/integration/setup.ts index 4d7190d7..09cc3a5c 100644 --- a/tests/integration/setup.ts +++ b/tests/integration/setup.ts @@ -118,11 +118,19 @@ export const createTestApp = (): TestContext => { return res.json({ success: true, data: [] }); } - // Simplified search: substring match scoped by database_membership + // Simplified search: substring match scoped by database_membership. + // The LEFT JOIN to vital_event mirrors production so the response shape + // (including birthYear) matches and contract regressions are caught. const results = db.prepare(` - SELECT p.person_id as personId, p.display_name as displayName, p.gender, p.birth_name as birthName + SELECT p.person_id as personId, p.display_name as displayName, p.gender, p.birth_name as birthName, ve.birth_year as birthYear FROM person p JOIN database_membership dm ON p.person_id = dm.person_id + LEFT JOIN ( + SELECT person_id, MIN(date_year) AS birth_year + FROM vital_event + WHERE event_type = 'birth' + GROUP BY person_id + ) ve ON ve.person_id = p.person_id WHERE dm.db_id = ? AND p.display_name LIKE ? ORDER BY p.display_name, p.person_id LIMIT 20 From 38befae649a0fbce9f228395051e7cdbcae651d3 Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Mon, 6 Apr 2026 22:37:11 -0700 Subject: [PATCH 10/14] fix(relationships): guard against late results and mid-link unmount - doSearch: bump request id (and clear searching) when query falls below 2 chars, so a previously-issued >=2-char request resolving late cannot repopulate stale results - RelationshipModal: introduce safeClose() that no-ops while a link/create is in flight, and disable the X close button when linking. Backdrop click and X button no longer unmount the modal mid-await, preventing setState/onLinked from running on a dead component --- .../src/components/person/RelationshipModal.tsx | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/client/src/components/person/RelationshipModal.tsx b/client/src/components/person/RelationshipModal.tsx index 91dd4464..31099b06 100644 --- a/client/src/components/person/RelationshipModal.tsx +++ b/client/src/components/person/RelationshipModal.tsx @@ -76,7 +76,11 @@ export function RelationshipModal({ open, dbId, personId, initialType, onClose, const doSearch = useCallback(async (q: string) => { if (q.length < 2) { + // Invalidate any in-flight search so a late >=2-char response cannot + // repopulate results after the user has deleted back to <2 chars. + searchRequestIdRef.current += 1; setResults([]); + setSearching(false); return; } const requestId = ++searchRequestIdRef.current; @@ -134,11 +138,17 @@ export function RelationshipModal({ open, dbId, personId, initialType, onClose, if (!open) return null; const linking = linkingId !== null; + // Block close interactions while a link/create request is in flight so we + // can't unmount mid-await and run setState/onLinked on a dead component. + const safeClose = () => { + if (linking) return; + onClose(); + }; return (
e.target === e.currentTarget && onClose()} + onClick={(e) => e.target === e.currentTarget && safeClose()} >
@@ -147,8 +157,9 @@ export function RelationshipModal({ open, dbId, personId, initialType, onClose,

Add Relationship

{linkingId === r.personId && } From 6b62e9376444812c74729205ee85c62fc8722f4e Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Mon, 6 Apr 2026 23:05:27 -0700 Subject: [PATCH 14/14] fix(relationships): normalize quick-search query param req.query.q may be string | string[] | undefined; calling .trim() directly on the cast crashed with a 500 if a client sent multiple ?q= parameters. Normalize to the first value before length checks. --- server/src/routes/person.routes.ts | 4 +++- tests/integration/setup.ts | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/server/src/routes/person.routes.ts b/server/src/routes/person.routes.ts index 146b3452..4085fb44 100644 --- a/server/src/routes/person.routes.ts +++ b/server/src/routes/person.routes.ts @@ -31,7 +31,9 @@ personRoutes.get('/:dbId', async (req, res, next) => { // GET /api/persons/:dbId/quick-search?q=name // Must be registered before /:dbId/:personId to avoid route conflict personRoutes.get('/:dbId/quick-search', (req, res) => { - const q = (req.query.q as string || '').trim(); + // req.query.q may be string | string[] | undefined; normalize to first value + const rawQ = req.query.q; + const q = (Array.isArray(rawQ) ? rawQ[0] : rawQ || '').toString().trim(); if (!q || q.length < 2) { return res.json({ success: true, data: [] }); } diff --git a/tests/integration/setup.ts b/tests/integration/setup.ts index 52b7a79e..e17c88bf 100644 --- a/tests/integration/setup.ts +++ b/tests/integration/setup.ts @@ -113,7 +113,9 @@ export const createTestApp = (): TestContext => { // GET /api/persons/:dbId/quick-search - FTS-style autocomplete // Must be registered before /:dbId/:personId to avoid route conflict app.get('/api/persons/:dbId/quick-search', (req, res) => { - const q = ((req.query.q as string) || '').trim(); + // req.query.q may be string | string[] | undefined; normalize first + const rawQ = req.query.q; + const q = (Array.isArray(rawQ) ? rawQ[0] : rawQ || '').toString().trim(); if (!q || q.length < 2) { return res.json({ success: true, data: [] }); }