diff --git a/.changelog/NEXT.md b/.changelog/NEXT.md index 871cb23..77a969d 100644 --- a/.changelog/NEXT.md +++ b/.changelog/NEXT.md @@ -49,6 +49,7 @@ ## Fixed +- Search results now keep their alphabetical ordering. The batch person-loader (`getPersonsBatch`) re-orders rows back to the requested order, fixing a regression where SQLite's `WHERE person_id IN (...)` returned rows in table order and silently discarded the search query's `ORDER BY display_name` (so the default, unsorted search view appeared randomly ordered). - Platform comparison now treats equivalent place spellings as matches: "Dallas, Texas, USA" vs "Dallas, Texas, United States" (and U.S.A. / United States of America / state abbreviations like TX vs Texas, UK vs United Kingdom, etc.) — no longer flagged as `different`. Place containment is now suffix-based, so "Texas" no longer falsely matches "Texarkana" - Platform comparison now treats equivalent date formats as matches (e.g., "1979-07-31" vs "31 JUL 1979") - `isLegacyFormat` augmentation type guard no longer crashes on string/null input @@ -63,4 +64,5 @@ - Socket.IO: removed server (`socket.service.ts`, `socket.io` dep) and client (`socket.ts`, `useSocket.ts` hooks), replaced with synchronous API + existing SSE - Dead code: `TreeView` component, `ConnectionLine` component, `batchInsert()` (had SQL injection risk), `getCanonicalDbId()` identity function +- Dead `searchService.quickSearch` and `searchService.searchGlobal` methods — uncalled anywhere (the live quick-search route has its own inline SQL) and the last remaining N+1 person-load loops in the search service - Dead `/socket.io` Vite proxy config diff --git a/DONE.md b/DONE.md index 4526974..26516f4 100644 --- a/DONE.md +++ b/DONE.md @@ -2,6 +2,10 @@ Completed items archived from PLAN.md. For per-version release notes see `.changelog/`. For full phase histories see [docs/roadmap.md](./docs/roadmap.md). +## 2026-06-27 + +- **Search ordering regression fixed + search-service N+1 cleanup (PLAN "Next Up" #4)** — `getPersonsBatch` now re-indexes its results into the caller's requested order. SQLite's `WHERE person_id IN (...)` returns rows in table (rowid) order, so the live search path (`searchWithSqlite` → `getPersonsBatch`) was silently dropping its `ORDER BY display_name`, leaving the default search view unordered. Also removed the two dead, never-called N+1 service methods (`searchService.quickSearch`, `searchService.searchGlobal`) and the now-unused `idMappingService` import. New unit tests in `tests/unit/services/databaseBatch.spec.ts` guard the ordering and missing-id behavior. Deferred `externalId` parity for batch results to PLAN (no current consumer needs it). + ## 2026-05-01 - **File size guard** — `scripts/check-file-sizes.ts` + `npm run check:file-sizes` wired into CI's build job. Tracks the nine god-files called out in PLAN.md and fails when any exceeds its budget. Prevents the regression we measured in the Phase 15.14 follow-up. Lock-in mechanism: when a file is split, lower its limit in `FILE_LIMITS`. Includes unit tests in `tests/unit/scripts/checkFileSizes.spec.ts`. diff --git a/PLAN.md b/PLAN.md index ad5ac98..8400aab 100644 --- a/PLAN.md +++ b/PLAN.md @@ -11,7 +11,7 @@ For phase-by-phase implementation history, see [docs/roadmap.md](./docs/roadmap. 1. **Reverse god-file regression** — `PersonDetail.tsx` (1360), `ProviderDataTable.tsx` (1243), `database.service.ts` (1618), `auditor-agent.service.ts` (1233 — new), `multi-platform-comparison.service.ts` (1099), `api.ts` (1249), `VerticalFamilyView.tsx` (977), `favorites.service.ts` (872), `person.routes.ts` (1119). Extract `usePersonData` / `usePersonOverrides` hooks, `PhotoThumbnail` / `ComparisonCell` / `ProviderRow` sub-components, and split `database.service.ts` along entity lines. Split `auditor-agent.service.ts` into walker + per-check modules. 2. **Critical-path unit tests** — `credentials.service.ts` (encryption), `validation.ts` (input sanitization), `errorHandler.ts`, `requestTimeout.ts`, `augmentation.service.ts`, `auditor-agent.service.ts` all currently have **zero** tests. (`database.service.ts` and `search.service.ts` now have integration coverage but no unit tests of internal helpers.) 3. **Phase 18 remaining checks** — implement `place_mismatch`, `name_mismatch`, `missing_parents`, `duplicate_suspect`, `stale_record` checks in `auditor-agent.service.ts` (types are declared in `shared/src/index.ts:918` but not yet wired). Reuse `multi-platform-comparison.service.ts` for the `*_mismatch` family. -4. **Search N+1** — `searchWithSqlite` still calls `getPerson()` per result inside `Promise.all`. Replace with a single `WHERE person_id IN (...)` batch query. +4. ~~**Search N+1**~~ — done. `searchWithSqlite` now uses `getPersonsBatch()` (6 queries vs 7×N), the batch loader preserves caller order (was silently dropping `ORDER BY display_name` because SQLite `IN (...)` returns table order), and the two dead N+1 service methods (`quickSearch`, `searchGlobal`) were removed. _Deferred:_ `getPersonsBatch()` does not populate `externalId` the way `getPerson()` does — fine for current consumers (SearchPage doesn't render it), but restore parity (one batched `external_identity` query) if a batch consumer ever needs it. 5. **Phase 19 Guided Verification** — review-session schema (`verification_session`, `person_review`, `edge_review`, `provider_match_review`, `review_decision`) and root-to-ancestor BFS review queue (19.1 + 19.2). ## Backlog diff --git a/server/src/services/database.service.ts b/server/src/services/database.service.ts index bad1283..1fcc179 100644 --- a/server/src/services/database.service.ts +++ b/server/src/services/database.service.ts @@ -396,10 +396,17 @@ function buildPersonsBatch(personIds: string[]): PersonWithId[] { claimMap.set(claim.person_id, arr); } + // SQLite's `WHERE person_id IN (...)` returns rows in table (rowid) order, not + // in the order of the IN list. Index the rows by id and assemble by iterating + // `personIds`, so the caller's ordering (e.g. search's `ORDER BY display_name`) + // is preserved; ids with no matching row are skipped. + const rowById = new Map(persons.map((r) => [r.person_id, r])); + // Assemble PersonWithId results const results: PersonWithId[] = []; - for (const row of persons) { - const pid = row.person_id; + for (const pid of personIds) { + const row = rowById.get(pid); + if (!row) continue; const events = vitalMap.get(pid); const birth = events?.get('birth'); const death = events?.get('death'); diff --git a/server/src/services/search.service.ts b/server/src/services/search.service.ts index d5844b8..062559b 100644 --- a/server/src/services/search.service.ts +++ b/server/src/services/search.service.ts @@ -1,7 +1,6 @@ import type { SearchParams, SearchResult, PersonWithId } from '@fsf/shared'; import { databaseService, resolveDbId } from './database.service.js'; import { sqliteService } from '../db/sqlite.service.js'; -import { idMappingService } from './id-mapping.service.js'; import { sanitizeFtsQuery } from '../utils/validation.js'; import { parseYear } from '../utils/parseYear.js'; @@ -279,100 +278,4 @@ export const searchService = { // Fall back to in-memory search return searchInMemory(dbId, params); }, - - /** - * Quick search by name only (optimized for autocomplete) - */ - async quickSearch( - dbId: string, - query: string, - limit: number = 10 - ): Promise { - // Resolve database ID to internal db_id - const internalDbId = resolveDbId(dbId); - - if (databaseService.isSqliteEnabled() && internalDbId && query.length >= 2) { - // Use FTS5 for fast prefix matching - const escapedQuery = sanitizeFtsQuery(query); - const personIds = sqliteService.queryAll<{ person_id: string }>( - `SELECT DISTINCT p.person_id - FROM person p - JOIN database_membership dm ON p.person_id = dm.person_id - JOIN person_fts fts ON p.person_id = fts.person_id - WHERE dm.db_id = @dbId - AND fts.display_name MATCH @query - LIMIT @limit`, - { - dbId: internalDbId, - query: `"${escapedQuery}"*`, - limit, - } - ); - - const persons = await Promise.all( - personIds.map(({ person_id }) => databaseService.getPerson(dbId, person_id)) - ); - return persons.filter((p): p is PersonWithId => p !== null); - } - - // Fall back to simple prefix search - const db = await databaseService.getDatabase(dbId); - const lowerQuery = query.toLowerCase(); - - return Object.entries(db) - .filter(([, person]) => person.name?.toLowerCase().startsWith(lowerQuery)) - .slice(0, limit) - .map(([id, person]) => ({ id, ...person })); - }, - - /** - * Search across all databases (SQLite only) - */ - async searchGlobal( - params: SearchParams - ): Promise<{ dbId: string; results: PersonWithId[]; total: number }[]> { - if (!databaseService.isSqliteEnabled()) { - // Not supported without SQLite - return []; - } - - const { q, page = 1, limit = 20 } = params; - if (!q) return []; - - const escapedQuery = sanitizeFtsQuery(q); - const offset = (page - 1) * limit; - - // Search across all databases - const results = sqliteService.queryAll<{ db_id: string; person_id: string }>( - `SELECT DISTINCT dm.db_id, p.person_id - FROM person p - JOIN database_membership dm ON p.person_id = dm.person_id - JOIN person_fts fts ON p.person_id = fts.person_id - WHERE fts MATCH @query - ORDER BY dm.db_id, p.display_name - LIMIT @limit OFFSET @offset`, - { - query: `"${escapedQuery}"*`, - limit, - offset, - } - ); - - // Group by database - const grouped = new Map(); - for (const { db_id, person_id } of results) { - const person = await databaseService.getPerson(db_id, person_id); - if (person) { - const dbResults = grouped.get(db_id) ?? []; - dbResults.push(person); - grouped.set(db_id, dbResults); - } - } - - return Array.from(grouped.entries()).map(([dbId, persons]) => ({ - dbId, - results: persons, - total: persons.length, - })); - }, }; diff --git a/tests/unit/services/databaseBatch.spec.ts b/tests/unit/services/databaseBatch.spec.ts new file mode 100644 index 0000000..f176776 --- /dev/null +++ b/tests/unit/services/databaseBatch.spec.ts @@ -0,0 +1,82 @@ +/** + * Unit tests for databaseService.getPersonsBatch ordering. + * + * Regression guard: SQLite's `WHERE person_id IN (...)` returns rows in table + * (rowid) order, not in the order of the IN list. The search service relies on + * the batch loader to preserve the order it computed via `ORDER BY display_name`, + * so the loader must re-index results back into the caller's requested order. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +interface PersonRow { + person_id: string; + display_name: string; + birth_name: string | null; + gender: string | null; + living: number; + bio: string | null; +} + +// Rows the mocked `person` table query will return — deliberately in a different +// order than any caller would request, to prove the loader re-orders. +let personRows: PersonRow[] = []; + +const makeRow = (id: string, name: string): PersonRow => ({ + person_id: id, + display_name: name, + birth_name: null, + gender: 'unknown', + living: 0, + bio: null, +}); + +// Mock the SQLite layer: the base `person` query returns `personRows`; every +// other batch query (vital_event, parent_edge, spouse_edge, claim) returns []. +vi.mock('../../../server/src/db/sqlite.service.js', () => ({ + sqliteService: { + initDb: vi.fn(), + closeDb: vi.fn(), + getStats: vi.fn(() => ({ persons: 0 })), + queryOne: vi.fn(() => undefined), + queryAll: vi.fn((sql: string) => { + if (sql.includes('FROM person WHERE person_id IN')) return personRows; + return []; + }), + }, +})); + +// Local overrides are exercised elsewhere; make them a no-op here so the batch +// loader can run without a real DB. +vi.mock('../../../server/src/utils/applyOverrides.js', () => ({ + applyLocalOverrides: vi.fn(), +})); + +const { databaseService } = await import('../../../server/src/services/database.service.js'); + +describe('databaseService.getPersonsBatch', () => { + beforeEach(() => { + personRows = []; + }); + + it('returns persons in the caller-requested order, not SQLite table order', () => { + // Table order (what the mock returns) differs from the requested order. + personRows = [makeRow('P1', 'Alice'), makeRow('P2', 'Bob'), makeRow('P3', 'Carol')]; + + const result = databaseService.getPersonsBatch(['P3', 'P1', 'P2']); + + expect(result.map((p) => p.id)).toEqual(['P3', 'P1', 'P2']); + }); + + it('drops requested ids that have no matching person row', () => { + personRows = [makeRow('P1', 'Alice'), makeRow('P3', 'Carol')]; + + const result = databaseService.getPersonsBatch(['P3', 'MISSING', 'P1']); + + expect(result.map((p) => p.id)).toEqual(['P3', 'P1']); + }); + + it('returns an empty array for an empty id list', () => { + expect(databaseService.getPersonsBatch([])).toEqual([]); + }); +});