Keep search results ordered when batch-loading persons

.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

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`.

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

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');

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<PersonWithId[]> {
-    // 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<string, PersonWithId[]>();
-    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,
-    }));
-  },
 };

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([]);
+  });
+});