Conversation
…uses, 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
There was a problem hiding this comment.
Pull request overview
Implements Phase 15.20 “Relationship linking” for SparseTree by adding server endpoints to quick-search people, link/unlink relationships (with optional stub-person creation), and a new client modal workflow to link existing people or create+link new stubs from PersonDetail.
Changes:
- Server: add
createPersonStub()and new relationship/quick-search endpoints under/api/persons/:dbId/.... - Client: add
RelationshipModal(search + create tabs) and wire+ Addactions inPersonDetail. - Docs: mark roadmap phase 15.20 as complete.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/services/id-mapping.service.ts | Adds createPersonStub() to create minimal person records and update FTS. |
| server/src/routes/person.routes.ts | Adds /quick-search, /link-relationship, and /unlink-relationship endpoints. |
| client/src/services/api.ts | Adds API helpers for quick-search and relationship link/unlink. |
| client/src/components/person/RelationshipModal.tsx | New modal UI for linking existing people or creating+linking stubs. |
| client/src/components/person/PersonDetail.tsx | Integrates modal and adds “+ Add” actions for parents/spouses/children sections. |
| PLAN.md | Marks Phase 15.20 as completed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| `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' |
There was a problem hiding this comment.
The LEFT JOIN to vital_event can return multiple rows per person because vital_event allows multiple birth records per source (UNIQUE is (person_id, event_type, source)). This will duplicate people in quick-search results. Consider joining a subquery that selects a single birth year per person (e.g., MAX/MIN date_year) or aggregating with GROUP BY to ensure one row per person.
| `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' | |
| `SELECT p.person_id, p.display_name, p.gender, p.birth_name, ve.birth_year AS birth_year | |
| 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 |
| 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})`); | ||
| } |
There was a problem hiding this comment.
link-relationship ignores the dbId scoping entirely: it doesn’t verify that the source person (canonical) is a member of database_membership for :dbId, and when creating a stub it never inserts the new person_id into database_membership. This can lead to cross-database linking and to linked people not appearing in list/search views (which are membership-scoped). Consider (1) rejecting requests when canonical isn’t in database_membership for dbId, (2) ensuring targetId is also a member (or explicitly adding it, if that’s intended), and (3) inserting the stub into database_membership for dbId in the same transaction as edge creation.
| } | ||
| } else { | ||
| // Create a new person stub | ||
| const gender = newPerson.gender || (relationshipType === 'father' ? 'male' : relationshipType === 'mother' ? 'female' : 'unknown'); |
There was a problem hiding this comment.
When creating a stub, newPerson.gender is accepted as an arbitrary string and passed through to person.gender, but the DB column has a CHECK constraint limiting values to ('male','female','unknown'). Invalid input will throw a DB error and return a 500. Validate newPerson.gender against the allowed set (and/or coerce to 'unknown') before calling createPersonStub.
| const gender = newPerson.gender || (relationshipType === 'father' ? 'male' : relationshipType === 'mother' ? 'female' : 'unknown'); | |
| const requestedGender = typeof newPerson.gender === 'string' ? newPerson.gender.toLowerCase() : ''; | |
| const gender = | |
| requestedGender === 'male' || requestedGender === 'female' || requestedGender === 'unknown' | |
| ? requestedGender | |
| : relationshipType === 'father' | |
| ? 'male' | |
| : relationshipType === 'mother' | |
| ? 'female' | |
| : 'unknown'; |
| 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; | ||
| } |
There was a problem hiding this comment.
unlink-relationship also ignores dbId scoping: it will delete edges for any canonical person ID regardless of whether that person is in the requested database, and it doesn’t verify that targetId belongs to the same database. Add membership checks (and ideally scope deletions via JOIN to database_membership) to prevent cross-database modifications.
| 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); |
There was a problem hiding this comment.
createPersonStub performs a person insert and then updates the FTS table, but it doesn’t wrap these writes in a transaction. If updatePersonFts fails, you can end up with a person record missing from FTS (or a partially updated FTS entry). Consider wrapping the insert + FTS update in sqliteService.transaction(() => { ... }) to keep the stub creation atomic.
| 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); | |
| 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); | |
| }); |
| const data = await api.quickSearchPersons(dbId, q); | ||
| setResults(data.filter(r => r.personId !== personId)); | ||
| setSearching(false); |
There was a problem hiding this comment.
doSearch doesn’t handle request failures. If api.quickSearchPersons throws, searching will remain true and the error will be unhandled (potentially spamming the console and leaving the UI stuck in a loading state). Wrap the call in try/catch/finally, show a toast on error, and ensure setSearching(false) runs in finally.
| 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); | |
| } |
| 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); | ||
| } |
There was a problem hiding this comment.
On modal open you reset query/results/etc., but searching (and linkingId) aren’t reset. If the modal was previously closed mid-search, it can reopen showing the spinner even with an empty query. Consider resetting setSearching(false) and setLinkingId(null) in the open branch of the effect.
| 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 }) | ||
| } |
There was a problem hiding this comment.
linkRelationship/unlinkRelationship accept relationshipType: string, which loses type-safety relative to the RelationshipType union used by the UI and server-side validation. Consider typing this parameter as RelationshipType (or the same string literal union) to prevent accidental invalid values at compile time.
| onLinked={() => { | ||
| api.getPerson(dbId!, personId!).then(updated => { | ||
| setPerson(updated); | ||
| toast.success('Relationship linked'); | ||
| }); |
There was a problem hiding this comment.
onLinked triggers api.getPerson(...).then(...) without a .catch(). If the refresh fails, it will produce an unhandled promise rejection and the UI won’t give feedback. Consider adding error handling (toast + keep modal open or at least fail silently) and/or awaiting the refresh before showing success.
| onLinked={() => { | |
| api.getPerson(dbId!, personId!).then(updated => { | |
| setPerson(updated); | |
| toast.success('Relationship linked'); | |
| }); | |
| onLinked={async () => { | |
| try { | |
| const updated = await api.getPerson(dbId!, personId!); | |
| setPerson(updated); | |
| toast.success('Relationship linked'); | |
| } catch (error) { | |
| toast.error('Failed to refresh person after linking relationship'); | |
| } |
| onLinked={() => { | ||
| api.getPerson(dbId!, personId!).then(updated => { | ||
| setPerson(updated); | ||
| toast.success('Relationship linked'); | ||
| }); |
There was a problem hiding this comment.
After linking, you only refresh person via api.getPerson but you don’t refetch parentData/spouseData/childData (those are loaded only in the initial useEffect tied to [dbId, personId]). This means newly linked relatives may render as empty cards until a full page refresh/navigation. Consider reusing the existing “load person + family” logic (extract into a reloadPerson() helper) or trigger a re-fetch of the affected family member data after setPerson(updated).
| onLinked={() => { | |
| api.getPerson(dbId!, personId!).then(updated => { | |
| setPerson(updated); | |
| toast.success('Relationship linked'); | |
| }); | |
| onLinked={async () => { | |
| try { | |
| const updated = await api.getPerson(dbId!, personId!); | |
| setPerson(updated); | |
| toast.success('Relationship linked'); | |
| // Reload the page so the initial effect re-runs and refreshes family data | |
| window.location.reload(); | |
| } catch (error) { | |
| toast.error('Failed to refresh person after linking relationship'); | |
| } |
Automated PR created by PortOS Chief of Staff.
Summary
ich specific result is being linked (spinner per-row)
9. Remove unnecessary comments in RelationshipModal
Now let me check how
database_membershipis used for scoping in the search service.Now I'll fix all the issues. Let me start with the quick-search route and the server-side fixes.
Now fix the quick-search route to use
sanitizeFtsQuery,database_membershipscoping, and a LEFT JOIN:Now replace the duplicated
validTypeswith the module-level constant:Now fix the RelationshipModal - collapse state, add error handling, fix debounce cleanup, fix spinner per-row, remove unnecessary comments:
Now collapse the two state variables in PersonDetail into one:
Now update all references to these two variables:
Now update the RelationshipModal usage:
Now remove the unused
type { RelationshipType }import since we're now usingnullas the closed state:Now let me rebuild to verify everything compiles.
Build succeeds. Let me verify there are no remaining references to the old state variable:
All clean. Here's a summary of what was fixed from the /simplify review:
Fixed:
sanitizeFtsQueryfromutils/validation.tsinstead of weaker inline sanitization (prevents FTS5 operator injection)vital_eventdatabase_membershipscoping to quick-search so results are filtered to the requested database (was returning results from ALL databases)showRelationshipModal+relationshipType) into singlerelationshipModalType: RelationshipType | nulltoast.erroron failure (UI no longer gets stuck)VALID_RELATIONSHIP_TYPESconstant to avoid duplication between link/unlink routeslinkingIdinstead of a blanketlinkingbooleanSkipped (acceptable):
createPerson(out of scope of this feature, existing path goes through sqlite-writer)insertParentEdgehelper (the duplication is between different relationship semantics and only ~5 lines)Now let me commit this work.
Now let me update PLAN.md to reflect that Phase 15.20 is implemented.
Implementation complete. Here's a summary:
Phase 15.20: Relationship Linking
What was built
Server (3 new endpoints):
GET /api/persons/:dbId/quick-search?q=name- FTS5 search scoped to database with LEFT JOIN for birth years (single query, no N+1)POST /api/persons/:dbId/:personId/link-relationship- Link existing person or create new stub as parent/spouse/child, with duplicate and self-link preventionDELETE /api/persons/:dbId/:personId/unlink-relationship- Remove relationship edgesServer (new service method):
idMappingService.createPersonStub()- Creates a minimal person record without requiring an external provider identityClient:
RelationshipModalcomponent with two tabs:Quality improvements from /simplify
sanitizeFtsQueryutility (prevents FTS5 operator injection)database_membershipscoping (bug fix: was returning cross-database results)relationshipModalType: RelationshipType | null